diff --git a/.github/scripts/changed-files.sh b/.github/scripts/changed-files.sh index 9912ac3e9..a99804ede 100644 --- a/.github/scripts/changed-files.sh +++ b/.github/scripts/changed-files.sh @@ -12,5 +12,13 @@ fi if [ -z "$base_sha" ]; then git ls-files -- "$@" else - git diff --name-only --diff-filter=ACMRT "$base_sha" "$head_sha" -- "$@" + # BASE..HEAD is a two-dot TREE diff, but consumers scan the PR's *merge-ref + # checkout*. A branch that lags main's deletions therefore gets paths listed + # (present on the branch, deleted on main) that don't exist on disk — and + # scanners like semgrep hard-fail on nonexistent roots ("Invalid scanning + # root"). Keep only paths present in the checkout. + git diff --name-only --diff-filter=ACMRT "$base_sha" "$head_sha" -- "$@" \ + | while IFS= read -r f; do + if [ -e "$f" ]; then printf '%s\n' "$f"; fi + done fi diff --git a/src/app/actions/messages.ts b/src/app/actions/messages.ts index 8af3b4fee..a4d80f2a0 100644 --- a/src/app/actions/messages.ts +++ b/src/app/actions/messages.ts @@ -14,6 +14,7 @@ import { notifyNewMessage } from '@/lib/notifications'; import { sendPushNotification } from '@/lib/notifications/push'; import { createAdminClient } from '@/lib/supabase/admin'; import { logServerError } from '@/lib/server-error-logger'; +import { maybeCaptureRlsDenial } from '@/lib/admin/rls-denial'; import { resolveCoachTeamIdWithCookie } from '@/lib/golf/resolve-team-server'; import { getCoachTeamSwitchContext } from '@/lib/golf/resolve-team'; import { withAdminObserved } from '@/lib/admin/observed-action'; @@ -74,6 +75,7 @@ async function isBaseballMessageNotificationEnabled(conversationId: string): Pro * @param sport - The sport context (for revalidation paths) * @param createNotifications - Whether to create notifications for other participants (default: true) */ +// nosemgrep: helmv3-action-missing-revalidate -- realtime-subscribed messages UI; revalidatePath caused a full reload on every send (see note above the return) export async function sendMessage({ conversationId, content, @@ -148,6 +150,14 @@ export async function sendMessage({ hint: messageError.hint, }, }); + maybeCaptureRlsDenial(messageError, { + table: messagesTable, + verb: 'insert', + action: 'messages.sendMessage', + feature: sport === 'golf' ? 'messaging' : 'baseball_messages', + sport, + userId: user.id, + }); throw new Error(`Failed to send message: ${messageError.message}`); } @@ -205,7 +215,7 @@ export async function sendMessage({ // NOTE: Removed revalidatePath calls - messages page uses real-time subscriptions // Revalidation was causing unnecessary page reloads on every message send - // SEMGREP-ALLOW: realtime-subscribed messages UI; revalidate would cause reload loop + // (the nosemgrep suppression for this lives on the function declaration) return { success: true }; } catch (err) { @@ -305,6 +315,14 @@ export async function createConversation({ insertData, }, }); + maybeCaptureRlsDenial(convError, { + table: conversationsTable, + verb: 'insert', + action: 'messages.createConversation', + feature: sport === 'golf' ? 'messaging' : 'baseball_messages', + sport, + userId: user.id, + }); throw new Error(`Failed to create conversation: ${convError?.message || 'Unknown error'}`); } @@ -332,6 +350,14 @@ export async function createConversation({ userId: user.id, }, }); + maybeCaptureRlsDenial(participantsError, { + table: participantsTable, + verb: 'insert', + action: 'messages.createConversation', + feature: sport === 'golf' ? 'messaging' : 'baseball_messages', + sport, + userId: user.id, + }); await supabase.from(conversationsTable as any).delete().eq('id', conversationId); throw new Error(`Failed to add participants: ${participantsError.message}`); } @@ -375,6 +401,14 @@ export async function markMessagesAsRead({ if (participantError) { await logServerError(`[Messages] Failed to update last_read_at: ${participantError instanceof Error ? participantError.message : String(participantError)}`, { action: 'messages.markMessagesAsRead' }); + maybeCaptureRlsDenial(participantError, { + table: participantsTable, + verb: 'update', + action: 'messages.markMessagesAsRead', + feature: sport === 'golf' ? 'messaging' : 'baseball_messages', + sport, + userId: user.id, + }); throw new Error('Failed to mark messages as read'); } @@ -393,6 +427,14 @@ export async function markMessagesAsRead({ if (messagesError) { await logServerError(`[Messages] Failed to mark messages as read: ${messagesError instanceof Error ? messagesError.message : String(messagesError)}`, { action: 'messages.markMessagesAsRead' }); + maybeCaptureRlsDenial(messagesError, { + table: messagesTable, + verb: 'update', + action: 'messages.markMessagesAsRead', + feature: sport === 'golf' ? 'messaging' : 'baseball_messages', + sport, + userId: user.id, + }); } revalidatePath(`/${sport}/dashboard/messages/${conversationId}`); diff --git a/src/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx new file mode 100644 index 000000000..e39c751f5 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx @@ -0,0 +1,91 @@ +// ============================================================================= +// DecisionRoomPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// getDecisionRoomData (withBaseballAction) independently re-resolves auth, so +// a session that expires between the page's own supabase.auth.getUser() check +// and this call throws BaseballUnauthorizedError. Before this fix, that error +// propagated straight out of the Server Component render to error.tsx and the +// error tracker (Sentry/Vercel) — the same class of bug fixed on the +// scout-packet/preview and scout-packets pages. This test pins the fix: the +// unauthorized case now redirects to /baseball/login (preserving returnTo), +// while any OTHER thrown error (a real failure for a signed-in, authorized +// coach) still propagates so error.tsx keeps handling genuine failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getDecisionRoomData: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/decision-room', () => ({ + getDecisionRoomData: mocks.getDecisionRoomData, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/staff-decision-room/StaffDecisionRoomClient', () => ({ + StaffDecisionRoomClient: () => null, +})); + +import DecisionRoomPage from '../page'; + +describe('DecisionRoomPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the auth check and the data fetch', async () => { + mocks.getDecisionRoomData.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(DecisionRoomPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/decision-room'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getDecisionRoomData.mockRejectedValue(new Error('decision room query failed')); + + await expect(DecisionRoomPage()).rejects.toThrow('decision room query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the data fetch succeeds', async () => { + mocks.getDecisionRoomData.mockResolvedValue({ items: [] }); + + const element = await DecisionRoomPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/decision-room/page.tsx b/src/app/baseball/(dashboard)/dashboard/decision-room/page.tsx index fb6b390ea..e5ce6a9d0 100644 --- a/src/app/baseball/(dashboard)/dashboard/decision-room/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/decision-room/page.tsx @@ -29,6 +29,8 @@ import { redirect } from 'next/navigation'; import { createClient } from '@/lib/supabase/server'; import { getDecisionRoomData } from '@/app/baseball/actions/decision-room'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { StaffDecisionRoomClient } from '@/components/baseball/staff-decision-room/StaffDecisionRoomClient'; import { fairwayScope } from '@/lib/redesign/flag'; @@ -50,9 +52,17 @@ export default async function DecisionRoomPage() { } // getDecisionRoomData resolves the active team + viewer capability and - // enforces auth/context server-side. Any failure (no active team, no coach - // role, missing capability) surfaces through error.tsx. - const data = await getDecisionRoomData(); + // enforces auth/context server-side, independently re-resolving auth + // (withBaseballAction). A session that expires in the narrow window between + // the check above and this call throws BaseballUnauthorizedError, which + // must redirect to login rather than raw-throw to error.tsx/Sentry. Any + // OTHER failure (no active team, no coach role, missing capability) is a + // genuine failure and keeps propagating to error.tsx. + const data = await redirectOnUnauthorized( + () => getDecisionRoomData(), + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/decision-room', + ); return (
diff --git a/src/app/baseball/(dashboard)/dashboard/operations/error.tsx b/src/app/baseball/(dashboard)/dashboard/operations/error.tsx new file mode 100644 index 000000000..a52a84013 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/operations/error.tsx @@ -0,0 +1,23 @@ +'use client'; + +import { RouteErrorBoundary } from '@/components/errors'; + +export default function Error({ + error, + reset, +}: { + error: Error & { digest?: string }; + reset: () => void; +}) { + return ( + + ); +} diff --git a/src/app/baseball/(dashboard)/dashboard/performance/builder/error.tsx b/src/app/baseball/(dashboard)/dashboard/performance/builder/error.tsx new file mode 100644 index 000000000..a5dda3046 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/performance/builder/error.tsx @@ -0,0 +1,23 @@ +'use client'; + +import { RouteErrorBoundary } from '@/components/errors'; + +export default function Error({ + error, + reset, +}: { + error: Error & { digest?: string }; + reset: () => void; +}) { + return ( + + ); +} diff --git a/src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/__tests__/page.test.tsx new file mode 100644 index 000000000..35301e49f --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/__tests__/page.test.tsx @@ -0,0 +1,108 @@ +// ============================================================================= +// CoachScoutPacketPreviewPage — BaseballUnauthorizedError must redirect, not +// raw-throw. +// +// getScoutPacketPreview (withBaseballAction) independently re-resolves auth, +// so a session that expires between the page's own getActiveBaseballContext() +// check and this call throws BaseballUnauthorizedError. Before this fix, that +// error propagated straight out of the Server Component render to error.tsx +// and the error tracker (Sentry/Vercel) — the same class of bug fixed on the +// baseball announcements page (ROOT CAUSE 3). This test pins the fix: the +// unauthenticated case now redirects to /baseball/login (preserving +// returnTo), while any OTHER thrown error (a real failure for a signed-in, +// authorized coach) still propagates so error.tsx keeps handling genuine +// failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getActiveBaseballContext: vi.fn(), + getScoutPacketPreview: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), + notFound: vi.fn(() => { + throw new Error('NOT_FOUND'); + }), +})); + +vi.mock('next/navigation', () => ({ + redirect: mocks.redirect, + notFound: mocks.notFound, +})); + +vi.mock('@/lib/baseball/active-context', () => ({ + getActiveBaseballContext: mocks.getActiveBaseballContext, +})); + +vi.mock('@/app/baseball/actions/scout-packet', () => ({ + getScoutPacketPreview: mocks.getScoutPacketPreview, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/passport/ScoutPacketView', () => ({ + ScoutPacketView: () => null, +})); + +import CoachScoutPacketPreviewPage from '../page'; + +const COACH_CONTEXT = { + activeTeamId: 'team-1', + activeRole: 'coach' as const, + activeCoachId: 'coach-1', + activePlayerId: null, +}; + +describe('CoachScoutPacketPreviewPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getActiveBaseballContext.mockResolvedValue(COACH_CONTEXT); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the context check and the preview fetch', async () => { + mocks.getScoutPacketPreview.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(CoachScoutPacketPreviewPage({ params: Promise.resolve({ id: 'player-1' }) })).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/players/player-1/scout-packet/preview'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getScoutPacketPreview.mockRejectedValue(new Error('assembly failed')); + + await expect( + CoachScoutPacketPreviewPage({ params: Promise.resolve({ id: 'player-1' }) }), + ).rejects.toThrow('assembly failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the preview fetch succeeds', async () => { + mocks.getScoutPacketPreview.mockResolvedValue({ ok: true }); + + const element = await CoachScoutPacketPreviewPage({ + params: Promise.resolve({ id: 'player-1' }), + }); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx b/src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx index 1bf523eab..7485e6236 100644 --- a/src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx @@ -13,6 +13,7 @@ import Link from 'next/link'; import { getActiveBaseballContext } from '@/lib/baseball/active-context'; import { getScoutPacketPreview } from '@/app/baseball/actions/scout-packet'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; import { ScoutPacketView } from '@/components/baseball/passport/ScoutPacketView'; import { IconArrowLeft } from '@/components/icons'; @@ -33,7 +34,27 @@ export default async function CoachScoutPacketPreviewPage({ params }: PageProps) if (context.activeRole !== 'coach') redirect('/baseball/player/passport'); // Capability is enforced inside getScoutPacketPreview (can_export_reports). - const model = await getScoutPacketPreview(id); + // + // getScoutPacketPreview independently re-resolves auth (withBaseballAction), + // so a session that expires in the narrow window between the context check + // above and this call throws BaseballUnauthorizedError here. Left uncaught, + // that raw-throws through this Server Component's render straight to + // error.tsx and the error tracker (Sentry/Vercel) instead of the honest + // "please sign in again" redirect — the same class of bug fixed on the + // baseball announcements page. Redirect on that specific case; any other + // thrown error (e.g. a real capability failure for a signed-in coach) is a + // genuine failure and should keep propagating to error.tsx. + let model: Awaited>; + try { + model = await getScoutPacketPreview(id); + } catch (error) { + if (error instanceof BaseballUnauthorizedError) { + redirect( + `/baseball/login?returnTo=${encodeURIComponent(`/baseball/dashboard/players/${id}/scout-packet/preview`)}`, + ); + } + throw error; + } return (
diff --git a/src/app/baseball/(dashboard)/dashboard/scout-packets/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/scout-packets/__tests__/page.test.tsx new file mode 100644 index 000000000..6a3c74760 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/scout-packets/__tests__/page.test.tsx @@ -0,0 +1,105 @@ +// ============================================================================= +// ScoutPacketsHubPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// getScoutPacketRoster (withBaseballAction) independently re-resolves auth, so +// a session that expires between the page's own getActiveBaseballContext() +// check and this call throws BaseballUnauthorizedError. Before this fix, that +// error propagated straight out of the Server Component render to error.tsx +// and the error tracker (Sentry/Vercel) — the same class of bug fixed on the +// baseball announcements page (ROOT CAUSE 3). This test pins the fix: the +// unauthenticated case now redirects to /baseball/login (preserving +// returnTo), while any OTHER thrown error (a real failure for a signed-in, +// authorized coach) still propagates so error.tsx keeps handling genuine +// failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getActiveBaseballContext: vi.fn(), + resolveBaseballCapabilities: vi.fn(), + getScoutPacketRoster: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/baseball/active-context', () => ({ + getActiveBaseballContext: mocks.getActiveBaseballContext, +})); + +vi.mock('@/lib/baseball/capabilities', () => ({ + resolveBaseballCapabilities: mocks.resolveBaseballCapabilities, +})); + +vi.mock('@/app/baseball/actions/scout-packet', () => ({ + getScoutPacketRoster: mocks.getScoutPacketRoster, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/passport/ScoutPacketsFairway', () => ({ + ScoutPacketsFairway: () => null, +})); + +import ScoutPacketsHubPage from '../page'; + +const COACH_CONTEXT = { + activeTeamId: 'team-1', + activeRole: 'coach' as const, + activeCoachId: 'coach-1', + activePlayerId: null, +}; + +describe('ScoutPacketsHubPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getActiveBaseballContext.mockResolvedValue(COACH_CONTEXT); + mocks.resolveBaseballCapabilities.mockResolvedValue({ + can_export_reports: true, + is_head_coach: false, + }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the context check and the roster fetch', async () => { + mocks.getScoutPacketRoster.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(ScoutPacketsHubPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=/baseball/dashboard/scout-packets', + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getScoutPacketRoster.mockRejectedValue(new Error('roster query failed')); + + await expect(ScoutPacketsHubPage()).rejects.toThrow('roster query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the roster fetch succeeds', async () => { + mocks.getScoutPacketRoster.mockResolvedValue({ players: [] }); + + const element = await ScoutPacketsHubPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx b/src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx index 21d8b921f..7e1cf6523 100644 --- a/src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx @@ -18,6 +18,7 @@ import { redirect } from 'next/navigation'; import { getActiveBaseballContext } from '@/lib/baseball/active-context'; import { resolveBaseballCapabilities } from '@/lib/baseball/capabilities'; import { getScoutPacketRoster } from '@/app/baseball/actions/scout-packet'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; import { ScoutPacketsFairway } from '@/components/baseball/passport/ScoutPacketsFairway'; import { fairwayScope } from '@/lib/redesign/flag'; @@ -35,7 +36,23 @@ export default async function ScoutPacketsHubPage() { redirect('/baseball/dashboard/command-center'); } - const roster = await getScoutPacketRoster(); + // getScoutPacketRoster independently re-resolves auth (withBaseballAction), + // so a session that expires in the narrow window between the checks above + // and this call throws BaseballUnauthorizedError here. Left uncaught, that + // raw-throws through this Server Component's render straight to error.tsx + // and the error tracker (Sentry/Vercel) instead of the honest "please sign + // in again" redirect — the same class of bug fixed on the baseball + // announcements page. Redirect on that specific case; any other thrown + // error (a real failure for a signed-in coach) keeps propagating to error.tsx. + let roster: Awaited>; + try { + roster = await getScoutPacketRoster(); + } catch (error) { + if (error instanceof BaseballUnauthorizedError) { + redirect('/baseball/login?returnTo=/baseball/dashboard/scout-packets'); + } + throw error; + } return (
diff --git a/src/app/baseball/(dashboard)/dashboard/scouting/error.tsx b/src/app/baseball/(dashboard)/dashboard/scouting/error.tsx new file mode 100644 index 000000000..f051e02c7 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/scouting/error.tsx @@ -0,0 +1,23 @@ +'use client'; + +import { RouteErrorBoundary } from '@/components/errors'; + +export default function Error({ + error, + reset, +}: { + error: Error & { digest?: string }; + reset: () => void; +}) { + return ( + + ); +} diff --git a/src/app/baseball/(dashboard)/dashboard/settings/audit/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/settings/audit/__tests__/page.test.tsx new file mode 100644 index 000000000..a65189e3b --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/settings/audit/__tests__/page.test.tsx @@ -0,0 +1,108 @@ +// ============================================================================= +// SettingsAuditPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// getProgramSettings and getSettingsAuditLog (both withBaseballAction) +// independently re-resolve auth, so a session that expires between the +// page's own supabase.auth.getUser() check and these calls throws +// BaseballUnauthorizedError. Before this fix, that error propagated straight +// out of the Server Component render to error.tsx and the error tracker +// (Sentry/Vercel) — the same class of bug fixed on the scout-packet/preview +// and scout-packets pages. This test pins the fix: the unauthorized case now +// redirects to /baseball/login (preserving returnTo) whether it surfaces on +// the FIRST or the SECOND sequential getter call, while any OTHER thrown +// error (a real failure for a signed-in, authorized coach) still propagates +// so error.tsx keeps handling genuine failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getProgramSettings: vi.fn(), + getSettingsAuditLog: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/program-settings', () => ({ + getProgramSettings: mocks.getProgramSettings, + getSettingsAuditLog: mocks.getSettingsAuditLog, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/settings/SettingsAuditClient', () => ({ + SettingsAuditClient: () => null, +})); + +import SettingsAuditPage from '../page'; + +describe('SettingsAuditPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired before the FIRST getter call', async () => { + mocks.getProgramSettings.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(SettingsAuditPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/audit'), + ); + expect(mocks.getSettingsAuditLog).not.toHaveBeenCalled(); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired before the SECOND getter call', async () => { + mocks.getProgramSettings.mockResolvedValue({ teamName: 'Rini U' }); + mocks.getSettingsAuditLog.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(SettingsAuditPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/audit'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getProgramSettings.mockResolvedValue({ teamName: 'Rini U' }); + mocks.getSettingsAuditLog.mockRejectedValue(new Error('audit log query failed')); + + await expect(SettingsAuditPage()).rejects.toThrow('audit log query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when both fetches succeed', async () => { + mocks.getProgramSettings.mockResolvedValue({ teamName: 'Rini U' }); + mocks.getSettingsAuditLog.mockResolvedValue([]); + + const element = await SettingsAuditPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/audit/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/audit/page.tsx index 32c243397..b8e085a28 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/audit/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/audit/page.tsx @@ -22,6 +22,8 @@ import { getProgramSettings, getSettingsAuditLog, } from '@/app/baseball/actions/program-settings'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { SettingsAuditClient } from '@/components/baseball/settings/SettingsAuditClient'; export const metadata = { @@ -38,8 +40,20 @@ export default async function SettingsAuditPage() { redirect('/baseball/login?returnTo=/baseball/dashboard/settings/audit'); } - const settings = await getProgramSettings(); - const entries = await getSettingsAuditLog(100); + // Both getters independently re-resolve auth (withBaseballAction). A + // session that expires in the narrow window between the check above and + // these calls throws BaseballUnauthorizedError, which must redirect to + // login rather than raw-throw to error.tsx/Sentry. Any OTHER failure (a + // real capability failure for a signed-in coach) keeps propagating. + const { settings, entries } = await redirectOnUnauthorized( + async () => { + const settings = await getProgramSettings(); + const entries = await getSettingsAuditLog(100); + return { settings, entries }; + }, + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/audit', + ); return ; } diff --git a/src/app/baseball/(dashboard)/dashboard/settings/imports/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/settings/imports/__tests__/page.test.tsx new file mode 100644 index 000000000..67fcafeed --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/settings/imports/__tests__/page.test.tsx @@ -0,0 +1,117 @@ +// ============================================================================= +// ImportSourcesPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// getProgramSettings and listImportSources (both withBaseballAction) +// independently re-resolve auth, so a session that expires between the +// page's own supabase.auth.getUser() check and these calls throws +// BaseballUnauthorizedError. Before this fix, that error propagated straight +// out of the Server Component render to error.tsx and the error tracker +// (Sentry/Vercel) — the same class of bug fixed on the scout-packet/preview +// and scout-packets pages. This test pins the fix: the unauthorized case now +// redirects to /baseball/login (preserving returnTo) whether it surfaces on +// the FIRST or the SECOND sequential getter call, while any OTHER thrown +// error (a real failure for a signed-in, authorized coach) still propagates +// so error.tsx keeps handling genuine failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getProgramSettings: vi.fn(), + listImportSources: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/program-settings', () => ({ + getProgramSettings: mocks.getProgramSettings, + listImportSources: mocks.listImportSources, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/settings/ImportSourcesClient', () => ({ + ImportSourcesClient: () => null, +})); + +import ImportSourcesPage from '../page'; + +describe('ImportSourcesPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired before the FIRST getter call', async () => { + mocks.getProgramSettings.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(ImportSourcesPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/imports'), + ); + expect(mocks.listImportSources).not.toHaveBeenCalled(); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired before the SECOND getter call', async () => { + mocks.getProgramSettings.mockResolvedValue({ + teamName: 'Rini U', + viewerCanManageImports: true, + }); + mocks.listImportSources.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(ImportSourcesPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/imports'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getProgramSettings.mockResolvedValue({ + teamName: 'Rini U', + viewerCanManageImports: true, + }); + mocks.listImportSources.mockRejectedValue(new Error('import sources query failed')); + + await expect(ImportSourcesPage()).rejects.toThrow('import sources query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when both fetches succeed', async () => { + mocks.getProgramSettings.mockResolvedValue({ + teamName: 'Rini U', + viewerCanManageImports: true, + }); + mocks.listImportSources.mockResolvedValue([]); + + const element = await ImportSourcesPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/imports/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/imports/page.tsx index 0df85d1db..12756c690 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/imports/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/imports/page.tsx @@ -22,6 +22,8 @@ import { getProgramSettings, listImportSources, } from '@/app/baseball/actions/program-settings'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { ImportSourcesClient } from '@/components/baseball/settings/ImportSourcesClient'; export const metadata = { @@ -41,8 +43,20 @@ export default async function ImportSourcesPage() { // Viewer caps drive edit affordances; the list is staff-only (the action // re-checks can_manage_imports server-side regardless of what the UI shows). - const settings = await getProgramSettings(); - const sources = await listImportSources(); + // Both getters independently re-resolve auth (withBaseballAction). A + // session that expires in the narrow window between the check above and + // these calls throws BaseballUnauthorizedError, which must redirect to + // login rather than raw-throw to error.tsx/Sentry. Any OTHER failure keeps + // propagating. + const { settings, sources } = await redirectOnUnauthorized( + async () => { + const settings = await getProgramSettings(); + const sources = await listImportSources(); + return { settings, sources }; + }, + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/imports', + ); return ( ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getProgramSettings: vi.fn(), + listIntegrations: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/program-settings', () => ({ + getProgramSettings: mocks.getProgramSettings, + listIntegrations: mocks.listIntegrations, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/settings/IntegrationsClient', () => ({ + IntegrationsClient: () => null, +})); + +import IntegrationsPage from '../page'; + +describe('IntegrationsPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired before the FIRST getter call', async () => { + mocks.getProgramSettings.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(IntegrationsPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/integrations'), + ); + expect(mocks.listIntegrations).not.toHaveBeenCalled(); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired before the SECOND getter call', async () => { + mocks.getProgramSettings.mockResolvedValue({ + teamName: 'Rini U', + viewerCanManageSettings: true, + }); + mocks.listIntegrations.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(IntegrationsPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/integrations'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getProgramSettings.mockResolvedValue({ + teamName: 'Rini U', + viewerCanManageSettings: true, + }); + mocks.listIntegrations.mockRejectedValue(new Error('integrations query failed')); + + await expect(IntegrationsPage()).rejects.toThrow('integrations query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when both fetches succeed', async () => { + mocks.getProgramSettings.mockResolvedValue({ + teamName: 'Rini U', + viewerCanManageSettings: true, + }); + mocks.listIntegrations.mockResolvedValue([]); + + const element = await IntegrationsPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/integrations/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/integrations/page.tsx index f76304acc..f04c67bac 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/integrations/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/integrations/page.tsx @@ -21,6 +21,8 @@ import { getProgramSettings, listIntegrations, } from '@/app/baseball/actions/program-settings'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { IntegrationsClient } from '@/components/baseball/settings/IntegrationsClient'; export const metadata = { @@ -38,8 +40,20 @@ export default async function IntegrationsPage() { redirect('/baseball/login?returnTo=/baseball/dashboard/settings/integrations'); } - const settings = await getProgramSettings(); - const integrations = await listIntegrations(); + // Both getters independently re-resolve auth (withBaseballAction). A + // session that expires in the narrow window between the check above and + // these calls throws BaseballUnauthorizedError, which must redirect to + // login rather than raw-throw to error.tsx/Sentry. Any OTHER failure keeps + // propagating. + const { settings, integrations } = await redirectOnUnauthorized( + async () => { + const settings = await getProgramSettings(); + const integrations = await listIntegrations(); + return { settings, integrations }; + }, + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/integrations', + ); return ( ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getPermissionMatrix: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/roles-permissions', () => ({ + getPermissionMatrix: mocks.getPermissionMatrix, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +import PermissionsPage from '../page'; + +describe('PermissionsPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the auth check and the matrix fetch', async () => { + mocks.getPermissionMatrix.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(PermissionsPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/permissions'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getPermissionMatrix.mockRejectedValue(new Error('permission matrix query failed')); + + await expect(PermissionsPage()).rejects.toThrow('permission matrix query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the matrix fetch succeeds', async () => { + mocks.getPermissionMatrix.mockResolvedValue({ groups: [], viewerCanInviteStaff: false }); + + const element = await PermissionsPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/permissions/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/permissions/page.tsx index dcbde351c..def3d37ae 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/permissions/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/permissions/page.tsx @@ -23,6 +23,8 @@ import { redirect } from 'next/navigation'; import { createClient } from '@/lib/supabase/server'; import { getPermissionMatrix } from '@/app/baseball/actions/roles-permissions'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { Card, CardContent, CardHeader } from '@/components/ui/card'; import { IconShield, IconLock, IconChevronRight, IconUsers } from '@/components/icons'; import { SectionMasthead, InkBadge } from '@/components/baseball/living-annual'; @@ -49,7 +51,16 @@ export default async function PermissionsPage() { redirect('/baseball/login?returnTo=/baseball/dashboard/settings/permissions'); } - const data = await getPermissionMatrix(); + // getPermissionMatrix independently re-resolves auth (withBaseballAction). + // A session that expires in the narrow window between the check above and + // this call throws BaseballUnauthorizedError, which must redirect to login + // rather than raw-throw to error.tsx/Sentry. Any OTHER failure keeps + // propagating. + const data = await redirectOnUnauthorized( + () => getPermissionMatrix(), + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/permissions', + ); return (
diff --git a/src/app/baseball/(dashboard)/dashboard/settings/program/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/settings/program/__tests__/page.test.tsx new file mode 100644 index 000000000..db2656541 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/settings/program/__tests__/page.test.tsx @@ -0,0 +1,91 @@ +// ============================================================================= +// ProgramSettingsPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// getProgramSettings (withBaseballAction) independently re-resolves auth, so +// a session that expires between the page's own supabase.auth.getUser() check +// and this call throws BaseballUnauthorizedError. Before this fix, that error +// propagated straight out of the Server Component render to error.tsx and the +// error tracker (Sentry/Vercel) — the same class of bug fixed on the +// scout-packet/preview and scout-packets pages. This test pins the fix: the +// unauthorized case now redirects to /baseball/login (preserving returnTo), +// while any OTHER thrown error (a real failure for a signed-in, authorized +// coach) still propagates so error.tsx keeps handling genuine failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getProgramSettings: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/program-settings', () => ({ + getProgramSettings: mocks.getProgramSettings, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/settings/ProgramSettingsClient', () => ({ + ProgramSettingsClient: () => null, +})); + +import ProgramSettingsPage from '../page'; + +describe('ProgramSettingsPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the auth check and the settings fetch', async () => { + mocks.getProgramSettings.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(ProgramSettingsPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/program'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getProgramSettings.mockRejectedValue(new Error('program settings query failed')); + + await expect(ProgramSettingsPage()).rejects.toThrow('program settings query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the settings fetch succeeds', async () => { + mocks.getProgramSettings.mockResolvedValue({ teamName: 'Rini U' }); + + const element = await ProgramSettingsPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/program/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/program/page.tsx index e5cbcc2aa..d2b9b75b6 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/program/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/program/page.tsx @@ -18,6 +18,8 @@ import { redirect } from 'next/navigation'; import { createClient } from '@/lib/supabase/server'; import { getProgramSettings } from '@/app/baseball/actions/program-settings'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { ProgramSettingsClient } from '@/components/baseball/settings/ProgramSettingsClient'; export const metadata = { @@ -36,8 +38,16 @@ export default async function ProgramSettingsPage() { } // Resolves active team + settings doc + viewer caps; enforces auth/context - // server-side. Failures (no active team, no coach) bubble to error.tsx. - const data = await getProgramSettings(); + // server-side (withBaseballAction), independently re-resolving auth. A + // session that expires in the narrow window between the check above and + // this call throws BaseballUnauthorizedError, which must redirect to login + // rather than raw-throw to error.tsx/Sentry. Any OTHER failure (no active + // team, no coach) keeps propagating to error.tsx. + const data = await redirectOnUnauthorized( + () => getProgramSettings(), + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/program', + ); return ; } diff --git a/src/app/baseball/(dashboard)/dashboard/settings/roles/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/settings/roles/__tests__/page.test.tsx new file mode 100644 index 000000000..b5362d1c2 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/settings/roles/__tests__/page.test.tsx @@ -0,0 +1,91 @@ +// ============================================================================= +// RolesPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// getRoleTemplates (withBaseballAction) independently re-resolves auth, so a +// session that expires between the page's own supabase.auth.getUser() check +// and this call throws BaseballUnauthorizedError. Before this fix, that error +// propagated straight out of the Server Component render to error.tsx and the +// error tracker (Sentry/Vercel) — the same class of bug fixed on the +// scout-packet/preview and scout-packets pages. This test pins the fix: the +// unauthorized case now redirects to /baseball/login (preserving returnTo), +// while any OTHER thrown error (a real failure for a signed-in, authorized +// coach) still propagates so error.tsx keeps handling genuine failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getRoleTemplates: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/roles-permissions', () => ({ + getRoleTemplates: mocks.getRoleTemplates, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +import RolesPage from '../page'; + +describe('RolesPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the auth check and the templates fetch', async () => { + mocks.getRoleTemplates.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(RolesPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/roles'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getRoleTemplates.mockRejectedValue(new Error('role templates query failed')); + + await expect(RolesPage()).rejects.toThrow('role templates query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the templates fetch succeeds', async () => { + mocks.getRoleTemplates.mockResolvedValue({ + programLabel: 'College', + roleTemplates: [], + viewerCanInviteStaff: false, + }); + + const element = await RolesPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/roles/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/roles/page.tsx index 33271e6e9..62e495935 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/roles/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/roles/page.tsx @@ -16,6 +16,8 @@ import { redirect } from 'next/navigation'; import { createClient } from '@/lib/supabase/server'; import { getRoleTemplates } from '@/app/baseball/actions/roles-permissions'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { Card, CardContent, CardHeader } from '@/components/ui/card'; import { IconUsers, IconChevronRight, IconShield } from '@/components/icons'; import { SectionMasthead } from '@/components/baseball/living-annual'; @@ -41,7 +43,16 @@ export default async function RolesPage() { redirect('/baseball/login?returnTo=/baseball/dashboard/settings/roles'); } - const data = await getRoleTemplates(); + // getRoleTemplates independently re-resolves auth (withBaseballAction). A + // session that expires in the narrow window between the check above and + // this call throws BaseballUnauthorizedError, which must redirect to login + // rather than raw-throw to error.tsx/Sentry. Any OTHER failure keeps + // propagating. + const data = await redirectOnUnauthorized( + () => getRoleTemplates(), + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/roles', + ); return (
diff --git a/src/app/baseball/(dashboard)/dashboard/settings/season/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/settings/season/__tests__/page.test.tsx new file mode 100644 index 000000000..3cc1e5f79 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/settings/season/__tests__/page.test.tsx @@ -0,0 +1,91 @@ +// ============================================================================= +// SeasonSettingsPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// listSeasons (withBaseballAction) independently re-resolves auth, so a +// session that expires between the page's own supabase.auth.getUser() check +// and this call throws BaseballUnauthorizedError. Before this fix, that error +// propagated straight out of the Server Component render to error.tsx and the +// error tracker (Sentry/Vercel) — the same class of bug fixed on the +// scout-packet/preview and scout-packets pages. This test pins the fix: the +// unauthorized case now redirects to /baseball/login (preserving returnTo), +// while any OTHER thrown error (a real failure for a signed-in, authorized +// coach) still propagates so error.tsx keeps handling genuine failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + listSeasons: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/team-season-settings', () => ({ + listSeasons: mocks.listSeasons, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/settings/SeasonSettingsClient', () => ({ + SeasonSettingsClient: () => null, +})); + +import SeasonSettingsPage from '../page'; + +describe('SeasonSettingsPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the auth check and the seasons fetch', async () => { + mocks.listSeasons.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(SeasonSettingsPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/season'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.listSeasons.mockRejectedValue(new Error('seasons query failed')); + + await expect(SeasonSettingsPage()).rejects.toThrow('seasons query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the seasons fetch succeeds', async () => { + mocks.listSeasons.mockResolvedValue({ seasons: [] }); + + const element = await SeasonSettingsPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/season/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/season/page.tsx index 3e5aed576..36aa5bf4e 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/season/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/season/page.tsx @@ -15,6 +15,8 @@ import { redirect } from 'next/navigation'; import { createClient } from '@/lib/supabase/server'; import { listSeasons } from '@/app/baseball/actions/team-season-settings'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { SeasonSettingsClient } from '@/components/baseball/settings/SeasonSettingsClient'; export const metadata = { @@ -32,7 +34,16 @@ export default async function SeasonSettingsPage() { redirect('/baseball/login?returnTo=/baseball/dashboard/settings/season'); } - const data = await listSeasons(); + // listSeasons independently re-resolves auth (withBaseballAction). A + // session that expires in the narrow window between the check above and + // this call throws BaseballUnauthorizedError, which must redirect to login + // rather than raw-throw to error.tsx/Sentry. Any OTHER failure keeps + // propagating. + const data = await redirectOnUnauthorized( + () => listSeasons(), + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/season', + ); return ; } diff --git a/src/app/baseball/(dashboard)/dashboard/settings/teams/__tests__/page.test.tsx b/src/app/baseball/(dashboard)/dashboard/settings/teams/__tests__/page.test.tsx new file mode 100644 index 000000000..7a7898f20 --- /dev/null +++ b/src/app/baseball/(dashboard)/dashboard/settings/teams/__tests__/page.test.tsx @@ -0,0 +1,91 @@ +// ============================================================================= +// TeamSettingsPage — BaseballUnauthorizedError must redirect, not raw-throw. +// +// getTeamJoinSettings (withBaseballAction) independently re-resolves auth, so +// a session that expires between the page's own supabase.auth.getUser() check +// and this call throws BaseballUnauthorizedError. Before this fix, that error +// propagated straight out of the Server Component render to error.tsx and the +// error tracker (Sentry/Vercel) — the same class of bug fixed on the +// scout-packet/preview and scout-packets pages. This test pins the fix: the +// unauthorized case now redirects to /baseball/login (preserving returnTo), +// while any OTHER thrown error (a real failure for a signed-in, authorized +// coach) still propagates so error.tsx keeps handling genuine failures. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ data: { user: { id: 'coach-1' } } })), + getTeamJoinSettings: vi.fn(), + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + })), +})); + +vi.mock('@/app/baseball/actions/team-season-settings', () => ({ + getTeamJoinSettings: mocks.getTeamJoinSettings, +})); + +// Mocked locally (not the real with-baseball-action module) so the page's +// `instanceof BaseballUnauthorizedError` check runs against the SAME class +// reference this test constructs errors with — no need to pull in the real +// module's Sentry/Supabase/capability dependency graph just to reach one +// error class. Defined inside vi.hoisted since vi.mock factories are hoisted +// above normal top-level declarations. +const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ + FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { + readonly status = 401; + constructor(message = 'You must be signed in.') { + super(message); + this.name = 'BaseballUnauthorizedError'; + } + }, +})); +vi.mock('@/lib/baseball/with-baseball-action', () => ({ + BaseballUnauthorizedError: FakeBaseballUnauthorizedError, +})); + +vi.mock('@/components/baseball/settings/TeamSettingsClient', () => ({ + TeamSettingsClient: () => null, +})); + +import TeamSettingsPage from '../page'; + +describe('TeamSettingsPage — BaseballUnauthorizedError redirects instead of raw-throwing', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getUser.mockResolvedValue({ data: { user: { id: 'coach-1' } } }); + }); + + it('redirects to /baseball/login (with returnTo) when the session expired between the auth check and the settings fetch', async () => { + mocks.getTeamJoinSettings.mockRejectedValue(new FakeBaseballUnauthorizedError()); + + await expect(TeamSettingsPage()).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/teams'), + ); + }); + + it('re-throws any OTHER error (a real failure for a signed-in, authorized coach) instead of redirecting', async () => { + mocks.getTeamJoinSettings.mockRejectedValue(new Error('team join settings query failed')); + + await expect(TeamSettingsPage()).rejects.toThrow('team join settings query failed'); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('renders normally when the settings fetch succeeds', async () => { + mocks.getTeamJoinSettings.mockResolvedValue({ joinCode: 'ABC123' }); + + const element = await TeamSettingsPage(); + expect(element).toBeTruthy(); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/baseball/(dashboard)/dashboard/settings/teams/page.tsx b/src/app/baseball/(dashboard)/dashboard/settings/teams/page.tsx index 35380f44f..2c9baef24 100644 --- a/src/app/baseball/(dashboard)/dashboard/settings/teams/page.tsx +++ b/src/app/baseball/(dashboard)/dashboard/settings/teams/page.tsx @@ -14,6 +14,8 @@ import { redirect } from 'next/navigation'; import { createClient } from '@/lib/supabase/server'; import { getTeamJoinSettings } from '@/app/baseball/actions/team-season-settings'; +import { BaseballUnauthorizedError } from '@/lib/baseball/with-baseball-action'; +import { redirectOnUnauthorized } from '@/lib/baseball/redirect-on-unauthorized'; import { TeamSettingsClient } from '@/components/baseball/settings/TeamSettingsClient'; export const metadata = { @@ -31,7 +33,16 @@ export default async function TeamSettingsPage() { redirect('/baseball/login?returnTo=/baseball/dashboard/settings/teams'); } - const data = await getTeamJoinSettings(); + // getTeamJoinSettings independently re-resolves auth (withBaseballAction). + // A session that expires in the narrow window between the check above and + // this call throws BaseballUnauthorizedError, which must redirect to login + // rather than raw-throw to error.tsx/Sentry. Any OTHER failure keeps + // propagating. + const data = await redirectOnUnauthorized( + () => getTeamJoinSettings(), + (error) => error instanceof BaseballUnauthorizedError, + '/baseball/dashboard/settings/teams', + ); return ; } diff --git a/src/app/baseball/actions/__tests__/lifting-v11-rls-denial.test.ts b/src/app/baseball/actions/__tests__/lifting-v11-rls-denial.test.ts new file mode 100644 index 000000000..be83d7f32 --- /dev/null +++ b/src/app/baseball/actions/__tests__/lifting-v11-rls-denial.test.ts @@ -0,0 +1,168 @@ +// @vitest-environment node +// ============================================================================= +// Regression test for the RLS-denial double-fire defect in logSetResult +// (src/app/baseball/actions/lifting-v11.ts). +// +// THE BUG: logSetResult captured the RLS denial explicitly at the swallow +// site (correct table `helm_lifting_set_results`) and then did `throw error` +// — rethrowing the RAW Postgrest-shaped error. Because logSetResult is itself +// wrapped in withBaseballAction, that raw error also propagated into the +// wrapper's own generic catch block (with-baseball-action.ts), which fires +// its own `maybeCaptureRlsDenial` fallback with `table: featureArea` +// ('baseball-lifting' — a feature-area slug, not a real table) since it still +// looked RLS-shaped (code 42501). A single denial thus produced TWO +// `RLS denial` admin_events: one correct, one bogus, polluting the per-table +// rls_denials feature-health rollup with a fake table name. +// +// THE FIX: rethrow a generic `BaseballActionError` (not the raw error) after +// the precise capture, matching watchlist.ts's addToWatchlist pattern — its +// message doesn't match `isRlsDenial`, so the wrapper's fallback no-ops. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(async () => ({ + data: { user: { id: 'user-1', email: 'player@example.com' } }, + })), + logServerEvent: vi.fn(async (..._args: unknown[]) => undefined), + logServerException: vi.fn(async (..._args: unknown[]) => undefined), + logServerError: vi.fn(async (..._args: unknown[]) => undefined), +})); + +vi.mock('@/lib/server-error-logger', () => ({ + logServerEvent: mocks.logServerEvent, + logServerException: mocks.logServerException, + logServerError: mocks.logServerError, +})); + +vi.mock('@sentry/nextjs', () => ({ + withScope: (fn: (scope: unknown) => unknown) => + fn({ setTag: vi.fn(), setUser: vi.fn(), addBreadcrumb: vi.fn() }), +})); + +vi.mock('next/cache', () => ({ revalidatePath: vi.fn() })); + +vi.mock('@/lib/baseball/active-context', () => ({ + getActiveBaseballContext: vi.fn(async () => ({ + userId: 'user-1', + activeTeamId: 'team-1', + activeRole: 'player' as const, + activeCoachId: null, + activePlayerId: 'player-1', + fellBackFromStale: false, + })), +})); + +vi.mock('@/lib/demo/baseball-config.server', () => ({ + isCurrentSessionBaseballDemo: vi.fn(async () => false), +})); + +// Bypass the player-access settings/capability resolution entirely (covered +// by player-access-action-gate.integration.test.ts) — this test is scoped to +// the RLS-denial capture wiring, not the access gate itself. +vi.mock('@/lib/baseball/player-access', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + requirePlayerAccess: vi.fn(async () => undefined), + }; +}); + +// Mutable per-test upsert error, referenced (not called) by the +// @/lib/supabase/server factory below — same pattern as +// player-access-action-gate.integration.test.ts's `toggleRow`/`staffCaps`. +let upsertError: { code?: string | null; message?: string | null } | null = null; + +vi.mock('@/lib/supabase/server', () => ({ + createClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser }, + from: vi.fn((table: string) => { + if (table === 'helm_lifting_sessions') { + return { + select: () => ({ + eq: () => ({ + maybeSingle: async () => ({ + data: { + id: 'session-1', + organization_id: 'org-1', + team_id: 'team-1', + athlete_id: 'athlete-1', + }, + }), + }), + }), + }; + } + if (table === 'helm_lifting_set_results') { + return { + upsert: () => ({ + select: () => ({ + single: async () => ({ data: null, error: upsertError }), + }), + }), + }; + } + throw new Error(`lifting-v11-rls-denial.test.ts: unexpected table "${table}"`); + }), + })), +})); + +import { logSetResult } from '@/app/baseball/actions/lifting-v11'; +import { BaseballActionError } from '@/lib/baseball/with-baseball-action'; + +const BASE_INPUT = { + sessionId: '11111111-1111-4111-8111-111111111111', + sessionExerciseId: '22222222-2222-4222-8222-222222222222', + setNumber: 1, + actualReps: 8, + actualLoad: 135, + loadUnit: 'lb', + rpe: 7, + velocity: null, + playerNote: null, +}; + +describe('logSetResult RLS-denial capture (regression: no double-fire)', () => { + beforeEach(() => { + vi.clearAllMocks(); + upsertError = { code: '42501', message: 'new row violates row-level security policy' }; + }); + + it('rejects with a sanitized BaseballActionError, not the raw Postgrest error', async () => { + await expect(logSetResult(BASE_INPUT)).rejects.toBeInstanceOf(BaseballActionError); + }); + + it('captures exactly ONE RLS denial event, with the precise table — not a second, bogus feature-area one from the wrapper fallback', async () => { + await expect(logSetResult(BASE_INPUT)).rejects.toBeInstanceOf(BaseballActionError); + + const rlsEvents = mocks.logServerEvent.mock.calls.filter( + (call): call is [string, ...unknown[]] => + typeof call[0] === 'string' && call[0].startsWith('RLS denial:'), + ); + + expect(rlsEvents).toHaveLength(1); + expect(rlsEvents[0]?.[0]).toBe('RLS denial: insert on helm_lifting_set_results'); + + // Pin the exact regression: the wrapper's generic catch-all fallback + // (with-baseball-action.ts) would fire this bogus event — table set to + // the feature-area slug, not a real table — if the raw error were + // rethrown instead of a BaseballActionError. + expect( + rlsEvents.some((call) => call[0] === 'RLS denial: rpc on baseball-lifting'), + ).toBe(false); + }); + + it('a non-RLS error still surfaces as a single unexpected-failure log (no RLS event, no regression to over-suppression)', async () => { + upsertError = { code: '23505', message: 'duplicate key value violates unique constraint' }; + + await expect(logSetResult(BASE_INPUT)).rejects.toBeInstanceOf(BaseballActionError); + + const rlsEvents = mocks.logServerEvent.mock.calls.filter( + (call): call is [string, ...unknown[]] => + typeof call[0] === 'string' && call[0].startsWith('RLS denial:'), + ); + expect(rlsEvents).toHaveLength(0); + expect(mocks.logServerException).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/app/baseball/actions/announcements.ts b/src/app/baseball/actions/announcements.ts index 8c6f01f67..b1273bd67 100644 --- a/src/app/baseball/actions/announcements.ts +++ b/src/app/baseball/actions/announcements.ts @@ -14,6 +14,7 @@ import { withAdminObserved } from '@/lib/admin/observed-action'; import { createClient } from '@/lib/supabase/server'; import { revalidatePath } from 'next/cache'; import { logServerError } from '@/lib/server-error-logger'; +import { maybeCaptureRlsDenial } from '@/lib/admin/rls-denial'; import { BaseballCapabilityError, requireBaseballCapability } from '@/lib/baseball/capabilities'; import { withBaseballAction, @@ -152,6 +153,13 @@ const createAnnouncementAction = withBaseballAction( .single(); if (annError || !announcement) { + maybeCaptureRlsDenial(annError, { + table: 'baseball_announcements', + verb: 'insert', + action: 'createAnnouncement', + feature: 'baseball_announcements', + sport: 'baseball', + }); return { success: false, error: 'Failed to create announcement' }; } @@ -194,7 +202,16 @@ async function getAnnouncementsWithMetaImpl( .eq('team_id', teamId) .order('published_at', { ascending: false }); - if (error) return { success: false, error: 'Failed to load announcements' }; + if (error) { + maybeCaptureRlsDenial(error, { + table: 'baseball_announcements', + verb: 'select', + action: 'getAnnouncementsWithMeta', + feature: 'baseball_announcements', + sport: 'baseball', + }); + return { success: false, error: 'Failed to load announcements' }; + } if (!announcements || announcements.length === 0) { return { success: true, data: [] }; } @@ -329,6 +346,13 @@ async function acknowledgeAnnouncementImpl( ); if (insertError) { + maybeCaptureRlsDenial(insertError, { + table: 'baseball_announcement_acknowledgements', + verb: 'insert', + action: 'acknowledgeAnnouncement', + feature: 'baseball_announcements', + sport: 'baseball', + }); return { success: false, error: 'Failed to acknowledge announcement' }; } @@ -393,7 +417,16 @@ const deleteAnnouncementAction = withBaseballAction( .delete() .eq('id', announcementId); - if (error) return { success: false, error: 'Failed to delete announcement' }; + if (error) { + maybeCaptureRlsDenial(error, { + table: 'baseball_announcements', + verb: 'delete', + action: 'deleteAnnouncement', + feature: 'baseball_announcements', + sport: 'baseball', + }); + return { success: false, error: 'Failed to delete announcement' }; + } revalidatePath(ANNOUNCEMENTS_PATH); return { success: true }; diff --git a/src/app/baseball/actions/games.ts b/src/app/baseball/actions/games.ts index a0517a5b4..ee223c631 100644 --- a/src/app/baseball/actions/games.ts +++ b/src/app/baseball/actions/games.ts @@ -4,6 +4,7 @@ import { withAdminObserved } from '@/lib/admin/observed-action'; import { createClient } from '@/lib/supabase/server'; import type { SupabaseClient } from '@supabase/supabase-js'; import { sanitizeDbError } from '@/lib/db-error'; +import { maybeCaptureRlsDenial } from '@/lib/admin/rls-denial'; import { revalidatePath } from 'next/cache'; import { parseCSV, @@ -929,7 +930,16 @@ const saveFullBoxScoreAction = withBaseballAction( p_opponent_score: opponentScore, }); - if (error) return { success: false, error: sanitizeDbError(error, 'games') }; + if (error) { + maybeCaptureRlsDenial(error, { + table: 'baseball_games', + verb: 'rpc', + action: 'saveFullBoxScore', + feature: 'baseball_games', + sport: 'baseball', + }); + return { success: false, error: sanitizeDbError(error, 'games') }; + } const result = data as { success?: boolean; error?: string } | null; if (!result?.success) { diff --git a/src/app/baseball/actions/lifting-v11.ts b/src/app/baseball/actions/lifting-v11.ts index 6d6d5911c..5124b3fc0 100644 --- a/src/app/baseball/actions/lifting-v11.ts +++ b/src/app/baseball/actions/lifting-v11.ts @@ -65,6 +65,7 @@ import { withBaseballAction, BaseballActionError, } from '@/lib/baseball/with-baseball-action'; +import { maybeCaptureRlsDenial } from '@/lib/admin/rls-denial'; import { appendLiftTimelineEvent } from '@/lib/baseball/timeline-writer'; import { appendGroupAudit, type GroupAuditEntry } from '@/lib/baseball/lifting/group-audit-writer'; import { @@ -2028,7 +2029,23 @@ export const logSetResult = withBaseballAction( ) .select('id') .single(); - if (error) throw error; + if (error) { + maybeCaptureRlsDenial(error, { + table: 'helm_lifting_set_results', + verb: 'insert', + action: 'logSetResult', + feature: 'baseball_lifting', + sport: 'baseball', + userId: ctx.user.id, + }); + // Rethrow a generic BaseballActionError (not the raw `error`) so the + // wrapper's catch-all `maybeCaptureRlsDenial` fallback in + // with-baseball-action.ts doesn't see an RLS-shaped error and + // double-fire a second, imprecise denial (table: 'baseball-lifting') + // on top of the precise one just captured above. Matches + // watchlist.ts's addToWatchlist pattern. + throw new BaseballActionError(); + } const setRowId = (data as { id: string }).id; // Auto-advance assigned -> started. diff --git a/src/app/baseball/actions/watchlist.ts b/src/app/baseball/actions/watchlist.ts index e5c8579e3..4e320839c 100644 --- a/src/app/baseball/actions/watchlist.ts +++ b/src/app/baseball/actions/watchlist.ts @@ -10,6 +10,7 @@ import { logSecurityEvent } from '@/lib/validation/server-action-validator'; import { notifyWatchlistAdd, notifyPipelineStageChange } from '@/lib/notifications'; import { WatchlistSchemas } from '@/lib/validation/action-schemas'; import { logServerError } from '@/lib/server-error-logger'; +import { maybeCaptureRlsDenial } from '@/lib/admin/rls-denial'; import { assertCoachCanRecruitPlayer } from '@/lib/baseball/recruitability'; import { BaseballCapabilityError } from '@/lib/baseball/capabilities'; import { @@ -134,6 +135,14 @@ const addToWatchlistAction = withBaseballAction( }); if (error) { + maybeCaptureRlsDenial(error, { + table: 'baseball_watchlists', + verb: 'insert', + action: 'addToWatchlist', + feature: 'baseball_watchlist', + sport: 'baseball', + userId: ctx.user.id, + }); throw new BaseballActionError(); } diff --git a/src/app/golf/actions/insights.ts b/src/app/golf/actions/insights.ts index 1bfec5500..a46149114 100644 --- a/src/app/golf/actions/insights.ts +++ b/src/app/golf/actions/insights.ts @@ -3733,7 +3733,21 @@ export async function dismissComposedInsight( */ async function triggerPlayerInsightsAfterRoundImpl( playerId: string -): Promise<{ success: boolean; insights_created?: number; error?: string; partial?: boolean }> { +): Promise<{ + success: boolean; + insights_created?: number; + error?: string; + partial?: boolean; + /** + * Stable, non-message-derived classification for the observability layer + * (observeActionSoftFailure / severityForSoftFailure in + * src/lib/admin/observe-action-result.ts). Set on the two `success: false` + * outcomes below that are routine, expected states — not incidents — so + * they're classified by this code instead of regex-matching the + * user-facing `error` string. + */ + code?: 'engine_no_recent_rounds' | 'engine_session_expired'; +}> { const startTime = Date.now(); // P0-04: track whether any mandatory generator failed so the caller // (postRoundTrigger) can mark the round PARTIAL rather than fully clean. @@ -3833,7 +3847,16 @@ async function triggerPlayerInsightsAfterRoundImpl( philosophyGate, }); } catch { - return { success: false, error: 'Player analysis failed (likely session expired in background context)' }; + // Expected in fire-and-forget/cron contexts (roster-sweep, post-round + // trigger) — analyzePlayer()'s user-scoped client has no session to + // scope to once the originating request has finished. `code` lets the + // observability layer classify this without regex-matching the + // message (see EXPECTED_SOFT_FAILURE_CODES in observe-action-result.ts). + return { + success: false, + error: 'Player analysis failed (likely session expired in background context)', + code: 'engine_session_expired', + }; } if (!analysis) { @@ -3856,9 +3879,14 @@ async function triggerPlayerInsightsAfterRoundImpl( // real failure, but the caller (analyze-player API) treats it as 200 // since there's no actual error to surface to ops. The dashboard // consumer uses optional chaining and degrades gracefully. + // `code: 'engine_no_recent_rounds'` marks this as an expected + // empty-state for observeActionSoftFailure (not a regex on this + // message) — the nightly roster-sweep cron hits this constantly for + // inactive players and it must stay out of the Errors tab / Sentry. return { success: false, error: 'No completed rounds in the last 90 days yet — insights will populate after the next round', + code: 'engine_no_recent_rounds', }; } diff --git a/src/hooks/useAdminRealtime.ts b/src/hooks/useAdminRealtime.ts index 317642359..57bd060c3 100644 --- a/src/hooks/useAdminRealtime.ts +++ b/src/hooks/useAdminRealtime.ts @@ -187,50 +187,14 @@ export function useAdminRealtime(options: UseAdminRealtimeOptions = {}): UseAdmi }, handleNewEvent ) - // Also listen for client errors as fallback - .on( - 'postgres_changes', - { - event: 'INSERT', - schema: 'public', - table: 'admin_client_errors', - }, - (payload) => { - // Transform client error to AdminEvent format - const clientError = payload.new as { - id: string; - error_message: string; - error_stack: string | null; - page_url: string | null; - user_id: string | null; - user_agent: string | null; - metadata: Record | null; - created_at: string; - }; - - const event: AdminEvent = { - id: clientError.id, - event_type: 'client_error', - severity: 'error', - title: 'Client Error', - message: clientError.error_message, - metadata: clientError.metadata, - user_id: clientError.user_id, - user_email: null, - url: clientError.page_url, - stack_trace: clientError.error_stack, - browser_info: clientError.user_agent ? { userAgent: clientError.user_agent } : null, - resolved: false, - resolved_at: null, - resolved_by: null, - created_at: clientError.created_at, - }; - - if (shouldIncludeEvent(event)) { - setEvents(prev => [event, ...prev].slice(0, maxEvents)); - } - } - ) + // NOTE: admin_client_errors was a prior-architecture table for client + // error capture, superseded when the client-error path was + // consolidated into error_logs + admin_events (the same pair the + // server-side logger writes to — see server-error-logger.ts and + // src/app/api/log-error/route.ts). Nothing writes to + // admin_client_errors anymore, so the realtime fallback subscription + // that used to live here was permanently inert; removed rather than + // kept as dead weight a future on-call could mistake for a live path. // Listen for API errors .on( 'postgres_changes', diff --git a/src/lib/admin/__tests__/observe-action-result.test.ts b/src/lib/admin/__tests__/observe-action-result.test.ts index 80669eca7..bae709945 100644 --- a/src/lib/admin/__tests__/observe-action-result.test.ts +++ b/src/lib/admin/__tests__/observe-action-result.test.ts @@ -2,15 +2,18 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; const mocks = vi.hoisted(() => ({ logServerError: vi.fn(async () => {}), + logServerEvent: vi.fn(async () => {}), })); vi.mock('@/lib/server-error-logger', () => ({ logServerError: mocks.logServerError, + logServerEvent: mocks.logServerEvent, })); import { extractActionSoftFailure, isExpectedSoftFailureMessage, + isExpectedEmptyStateCode, observeActionSoftFailure, } from '@/lib/admin/observe-action-result'; import { __resetEmitThrottleForTests } from '@/lib/admin/emit-throttle'; @@ -32,6 +35,19 @@ describe('observe-action-result', () => { }); }); + it('extracts the stable `code` field alongside the message', () => { + expect( + extractActionSoftFailure({ + success: false, + error: 'No completed rounds in the last 90 days yet — insights will populate after the next round', + code: 'engine_no_recent_rounds', + }), + ).toEqual({ + message: 'No completed rounds in the last 90 days yet — insights will populate after the next round', + code: 'engine_no_recent_rounds', + }); + }); + it('extracts { data: null, error } envelopes', () => { expect(extractActionSoftFailure({ data: null, error: 'missing row' })).toEqual({ message: 'missing row', @@ -44,6 +60,19 @@ describe('observe-action-result', () => { expect(isExpectedSoftFailureMessage('Could not complete the calendar action. Please try again.')).toBe(false); }); + it('classifies the engine_session_expired code as expected regardless of message wording', () => { + // Structural, not message-string matching: an arbitrary message paired + // with the known code is still expected. + expect(isExpectedSoftFailureMessage('anything at all', 'engine_session_expired')).toBe(true); + expect(isExpectedSoftFailureMessage('anything at all', 'some_other_code')).toBe(false); + }); + + it('classifies engine_no_recent_rounds as an empty-state code, not a generic soft failure', () => { + expect(isExpectedEmptyStateCode('engine_no_recent_rounds')).toBe(true); + expect(isExpectedEmptyStateCode('engine_session_expired')).toBe(false); + expect(isExpectedEmptyStateCode(null)).toBe(false); + }); + it('logs unexpected soft failures at error severity', () => { observeActionSoftFailure( { success: false, error: 'Could not save document' }, @@ -78,4 +107,61 @@ describe('observe-action-result', () => { ); expect(mocks.logServerError).not.toHaveBeenCalled(); }); + + // ROOT CAUSE 2 — "No completed rounds in the last 90 days yet..." from + // triggerPlayerInsightsAfterRound during the roster-sweep cron must never + // land in admin_events at 'error' severity or as a Sentry exception; it is + // an expected empty state, classified via `code`, not the message text. + it('logs the no-recent-rounds empty state via logServerEvent at info severity, not logServerError', () => { + observeActionSoftFailure( + { + success: false, + error: 'No completed rounds in the last 90 days yet — insights will populate after the next round', + code: 'engine_no_recent_rounds', + }, + { action: 'triggerPlayerInsightsAfterRound', sport: 'golf', feature: 'coachhelm_ai_engine', source: 'server_action' }, + ); + + expect(mocks.logServerError).not.toHaveBeenCalled(); + expect(mocks.logServerEvent).toHaveBeenCalledTimes(1); + const infoCall = mocks.logServerEvent.mock.calls[0] as + | [string, Record | undefined, 'info' | 'warning' | 'error' | 'critical'] + | undefined; + expect(infoCall?.[2]).toBe('info'); + expect(infoCall?.[1]).toMatchObject({ skipSentry: true }); + }); + + it('logs an engine session-expired background failure as warning with skipSentry, not error', () => { + observeActionSoftFailure( + { + success: false, + error: 'Player analysis failed (likely session expired in background context)', + code: 'engine_session_expired', + }, + { action: 'triggerPlayerInsightsAfterRound', sport: 'golf', feature: 'coachhelm_ai_engine', source: 'server_action' }, + ); + + expect(mocks.logServerEvent).not.toHaveBeenCalled(); + expect(mocks.logServerError).toHaveBeenCalledTimes(1); + const warningCall = mocks.logServerError.mock.calls[0] as + | [string, Record | undefined, 'warning' | 'error' | 'critical'] + | undefined; + expect(warningCall?.[2]).toBe('warning'); + expect(warningCall?.[1]).toMatchObject({ skipSentry: true }); + }); + + it('still logs a real failure at error severity even when it carries an unrecognized code', () => { + observeActionSoftFailure( + { success: false, error: 'Database connection pool exhausted', code: 'db_pool_exhausted' }, + { action: 'someOtherAction', sport: 'golf', source: 'server_action' }, + ); + + expect(mocks.logServerEvent).not.toHaveBeenCalled(); + expect(mocks.logServerError).toHaveBeenCalledTimes(1); + const errorCall = mocks.logServerError.mock.calls[0] as + | [string, Record | undefined, 'warning' | 'error' | 'critical'] + | undefined; + expect(errorCall?.[2]).toBe('error'); + expect(errorCall?.[1]).toMatchObject({ skipSentry: false }); + }); }); diff --git a/src/lib/admin/observe-action-result.ts b/src/lib/admin/observe-action-result.ts index d5368873e..49bb990bc 100644 --- a/src/lib/admin/observe-action-result.ts +++ b/src/lib/admin/observe-action-result.ts @@ -2,7 +2,7 @@ import 'server-only'; import { isExpectedAuthNoise } from '@/lib/admin/data/triage'; import { drainCollapsedCount, shouldEmit } from '@/lib/admin/emit-throttle'; -import { logServerError } from '@/lib/server-error-logger'; +import { logServerError, logServerEvent } from '@/lib/server-error-logger'; export type ActionSoftFailureContext = NonNullable[1]> & { action: string; @@ -22,6 +22,37 @@ const EXPECTED_SOFT_FAILURE_PATTERNS: readonly RegExp[] = [ /^please (enter|select|provide)/i, ]; +/** + * Structural (code-based) counterpart to EXPECTED_SOFT_FAILURE_PATTERNS. + * Prefer giving an action's `{ success: false }` envelope a stable `code` + * over adding a new message regex above — codes survive copy edits to the + * user-facing string, and don't require this shared module to know every + * caller's exact wording. Same severity bucket as the message patterns + * ('warning', skipSentry): still a soft failure, just an expected one. + */ +const EXPECTED_SOFT_FAILURE_CODES: ReadonlySet = new Set([ + // Fire-and-forget / cron engine calls run without a user session by + // design (triggerPlayerInsightsAfterRound's analyzePlayer() call uses a + // user-scoped client that has nothing to scope to once the request that + // spawned the background work has completed). Same class as the + // "not authenticated" patterns above, just engine-side. + 'engine_session_expired', +]); + +/** + * Genuinely empty, nothing-failed outcomes — there is simply no data yet + * (e.g. a player with no completed rounds in the lookback window). Distinct + * from EXPECTED_SOFT_FAILURE_CODES: those are still benign soft *failures*; + * these were never a failure to begin with. Classified one tier quieter + * ('info', skipSentry) so they land in job telemetry without counting + * toward the Errors tab / Sentry — see admin/data/incident-feed.ts (default + * view excludes only 'info') and admin/data/overview.ts (KPI counts key off + * severity IN ('error','critical')). + */ +const EXPECTED_EMPTY_STATE_CODES: ReadonlySet = new Set([ + 'engine_no_recent_rounds', +]); + export function extractActionSoftFailure( result: unknown, ): { message: string; code: string | null } | null { @@ -58,13 +89,22 @@ export function extractActionSoftFailure( return null; } -export function isExpectedSoftFailureMessage(message: string): boolean { +export function isExpectedSoftFailureMessage(message: string, code?: string | null): boolean { + if (code && EXPECTED_SOFT_FAILURE_CODES.has(code)) return true; if (isExpectedAuthNoise(message)) return true; return EXPECTED_SOFT_FAILURE_PATTERNS.some((pattern) => pattern.test(message.trim())); } -function severityForSoftFailure(message: string): 'warning' | 'error' { - return isExpectedSoftFailureMessage(message) ? 'warning' : 'error'; +/** True for a stable `code` marking a routine empty-state, not a failure. */ +export function isExpectedEmptyStateCode(code: string | null): boolean { + return code != null && EXPECTED_EMPTY_STATE_CODES.has(code); +} + +type SoftFailureSeverity = 'info' | 'warning' | 'error'; + +function severityForSoftFailure(message: string, code: string | null): SoftFailureSeverity { + if (isExpectedEmptyStateCode(code)) return 'info'; + return isExpectedSoftFailureMessage(message, code) ? 'warning' : 'error'; } /** @@ -82,29 +122,35 @@ export function observeActionSoftFailure( if (!shouldEmit(throttleKey)) return; const collapsedCount = drainCollapsedCount(throttleKey); - const expected = isExpectedSoftFailureMessage(failure.message); - const severity = severityForSoftFailure(failure.message); + const severity = severityForSoftFailure(failure.message, failure.code); + // 'info' (expected empty-state) and 'warning' (expected soft failure) are + // both routine — never worth a Sentry capture. Only real 'error' outcomes + // get sent. + const skipSentry = severity !== 'error'; + const logContext = { + ...context, + title: `[${context.action}] ${failure.message}`.slice(0, 500), + handled: true, + skipSentry, + errorCode: failure.code ?? undefined, + fingerprint: ['server_action_soft', context.feature ?? context.featureArea ?? 'unknown', context.action], + metadata: { + ...(context.metadata ?? {}), + soft_failure: true, + ...(collapsedCount > 0 ? { collapsed_count: collapsedCount } : {}), + }, + }; try { - void Promise.resolve( - logServerError( - failure.message, - { - ...context, - title: `[${context.action}] ${failure.message}`.slice(0, 500), - handled: true, - skipSentry: expected, - errorCode: failure.code ?? undefined, - fingerprint: ['server_action_soft', context.feature ?? context.featureArea ?? 'unknown', context.action], - metadata: { - ...(context.metadata ?? {}), - soft_failure: true, - ...(collapsedCount > 0 ? { collapsed_count: collapsedCount } : {}), - }, - }, - severity, - ), - ).catch(() => {}); + // logServerError's severity param intentionally excludes 'info' (that + // tier belongs to logServerEvent) — route empty-state outcomes there so + // they land in admin_events at 'info' without widening logServerError's + // contract for every other caller of this shared capture class. + const capture = + severity === 'info' + ? logServerEvent(failure.message, logContext, 'info') + : logServerError(failure.message, logContext, severity); + void Promise.resolve(capture).catch(() => {}); } catch { // Fire-and-forget: observability must never change action results. } diff --git a/src/lib/baseball/__tests__/redirect-on-unauthorized.test.ts b/src/lib/baseball/__tests__/redirect-on-unauthorized.test.ts new file mode 100644 index 000000000..6dc37ab36 --- /dev/null +++ b/src/lib/baseball/__tests__/redirect-on-unauthorized.test.ts @@ -0,0 +1,91 @@ +// ============================================================================= +// redirectOnUnauthorized — shared guard used by every Server Component page +// that calls a withBaseballAction-wrapped getter directly (decision-room, +// settings/audit, settings/imports, settings/integrations, +// settings/permissions, settings/program, settings/roles, settings/season, +// settings/teams — and the previously-fixed scout-packet/preview and +// scout-packets pages). Pins the three behaviors every caller relies on: +// 1. isUnauthorized(error) === true -> redirect(returnTo), never resolves. +// 2. isUnauthorized(error) === false -> rethrows the ORIGINAL error, no redirect. +// 3. action() succeeds -> returns the value, no redirect. +// ============================================================================= + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + redirect: vi.fn((path: string) => { + throw new Error(`REDIRECT:${path}`); + }), +})); + +vi.mock('next/navigation', () => ({ redirect: mocks.redirect })); + +import { redirectOnUnauthorized } from '../redirect-on-unauthorized'; + +class FakeUnauthorizedError extends Error {} + +describe('redirectOnUnauthorized', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('redirects to the given returnTo (URL-encoded) when isUnauthorized matches the thrown error', async () => { + const action = vi.fn(async () => { + throw new FakeUnauthorizedError('session expired'); + }); + + await expect( + redirectOnUnauthorized( + action, + (error) => error instanceof FakeUnauthorizedError, + '/baseball/dashboard/settings/program', + ), + ).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/settings/program'), + ); + expect(mocks.redirect).toHaveBeenCalledTimes(1); + }); + + it('URL-encodes query params already present in returnTo', async () => { + const action = vi.fn(async () => { + throw new FakeUnauthorizedError(); + }); + + await expect( + redirectOnUnauthorized( + action, + () => true, + '/baseball/dashboard/players/player-1/scout-packet/preview', + ), + ).rejects.toThrow( + 'REDIRECT:/baseball/login?returnTo=' + + encodeURIComponent('/baseball/dashboard/players/player-1/scout-packet/preview'), + ); + }); + + it('rethrows the original error unchanged when isUnauthorized returns false, without redirecting', async () => { + const realError = new Error('a genuine capability failure'); + const action = vi.fn(async () => { + throw realError; + }); + + await expect( + redirectOnUnauthorized(action, () => false, '/baseball/dashboard/settings/program'), + ).rejects.toBe(realError); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); + + it('returns the resolved value when action succeeds, without redirecting', async () => { + const action = vi.fn(async () => ({ ok: true as const })); + + const result = await redirectOnUnauthorized( + action, + () => true, + '/baseball/dashboard/settings/program', + ); + + expect(result).toEqual({ ok: true }); + expect(mocks.redirect).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/baseball/__tests__/with-baseball-action-observability.test.ts b/src/lib/baseball/__tests__/with-baseball-action-observability.test.ts index 4e453392f..ab3dd1dc3 100644 --- a/src/lib/baseball/__tests__/with-baseball-action-observability.test.ts +++ b/src/lib/baseball/__tests__/with-baseball-action-observability.test.ts @@ -15,6 +15,7 @@ const mocks = vi.hoisted(() => ({ isCurrentSessionBaseballDemo: vi.fn(async () => false), logServerException: vi.fn(async (..._args: unknown[]) => undefined), logServerError: vi.fn(async (..._args: unknown[]) => undefined), + logServerEvent: vi.fn(async (..._args: unknown[]) => undefined), scope: { setTag: vi.fn(), setUser: vi.fn(), @@ -39,6 +40,7 @@ vi.mock('@/lib/demo/baseball-config.server', () => ({ vi.mock('@/lib/server-error-logger', () => ({ logServerException: mocks.logServerException, logServerError: mocks.logServerError, + logServerEvent: mocks.logServerEvent, })); vi.mock('@sentry/nextjs', () => ({ @@ -50,6 +52,10 @@ import { BaseballNoActiveTeamError, withBaseballAction, } from '../with-baseball-action'; +// Intentionally NOT mocked (see the "RLS-denial capture" describe block +// below) — the real isRlsDenial/maybeCaptureRlsDenial logic is exercised +// against the (real) `logServerEvent` mock above. +import { maybeCaptureRlsDenial } from '@/lib/admin/rls-denial'; beforeEach(() => { vi.clearAllMocks(); @@ -182,3 +188,82 @@ describe('withBaseballAction observability', () => { }); }); }); + +// ============================================================================= +// Wrapper-level RLS-denial capture (with-baseball-action.ts:506-517). +// +// This branch was previously untested here: the module mock above didn't +// even export `logServerEvent` (the function `maybeCaptureRlsDenial` calls), +// so any RLS-shaped throw silently no-op'd inside `maybeCaptureRlsDenial`'s +// try/catch instead of asserting anything. `@/lib/admin/rls-denial` is +// intentionally left UNMOCKED below so the real `isRlsDenial` / +// `maybeCaptureRlsDenial` logic runs against our (now real) `logServerEvent` +// mock. +// ============================================================================= + +describe('withBaseballAction RLS-denial capture', () => { + it('fires the generic fallback (table=featureArea, verb=rpc) when the action body throws an RLS-shaped error directly', async () => { + const action = withBaseballAction( + 'recomputeDynamicGroup', + { featureArea: 'baseball-lifting', feature: 'baseball_lifting' }, + async () => { + throw { code: '42501', message: 'new row violates row-level security policy' }; + }, + ); + + await expect(action()).rejects.toBeInstanceOf(BaseballActionError); + + const rlsCalls = mocks.logServerEvent.mock.calls.filter( + (call) => typeof call[0] === 'string' && call[0].startsWith('RLS denial:'), + ); + expect(rlsCalls).toHaveLength(1); + expect(rlsCalls[0]?.[0]).toBe('RLS denial: rpc on baseball-lifting'); + }); + + it('does not double-fire when the action body already captured a precise denial and rethrows a generic BaseballActionError', async () => { + // Mirrors the fixed logSetResult pattern (lifting-v11.ts) and + // watchlist.ts's addToWatchlist: capture a precise denial at the swallow + // site, then rethrow a *generic* BaseballActionError (not the raw error) + // so the wrapper's own fallback below doesn't see an RLS-shaped error and + // fire a second, bogus (table=featureArea) denial on top of it. + const action = withBaseballAction( + 'logSetResult', + { featureArea: 'baseball-lifting', feature: 'baseball_lifting' }, + async () => { + maybeCaptureRlsDenial( + { code: '42501', message: 'new row violates row-level security policy' }, + { + table: 'helm_lifting_set_results', + verb: 'insert', + action: 'logSetResult', + sport: 'baseball', + }, + ); + throw new BaseballActionError(); + }, + ); + + await expect(action()).rejects.toBeInstanceOf(BaseballActionError); + + const rlsCalls = mocks.logServerEvent.mock.calls.filter( + (call) => typeof call[0] === 'string' && call[0].startsWith('RLS denial:'), + ); + expect(rlsCalls).toHaveLength(1); + expect(rlsCalls[0]?.[0]).toBe('RLS denial: insert on helm_lifting_set_results'); + expect(rlsCalls.some((call) => call[0] === 'RLS denial: rpc on baseball-lifting')).toBe(false); + }); + + it('does not fire for a non-RLS error (no over-suppression / false positives)', async () => { + const action = withBaseballAction( + 'someAction', + { featureArea: 'baseball-lifting', feature: 'baseball_lifting' }, + async () => { + throw new Error('unexpected null pointer'); + }, + ); + + await expect(action()).rejects.toBeInstanceOf(BaseballActionError); + expect(mocks.logServerEvent).not.toHaveBeenCalled(); + expect(mocks.logServerException).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/lib/baseball/redirect-on-unauthorized.ts b/src/lib/baseball/redirect-on-unauthorized.ts new file mode 100644 index 000000000..2e9729707 --- /dev/null +++ b/src/lib/baseball/redirect-on-unauthorized.ts @@ -0,0 +1,41 @@ +// ============================================================================= +// src/lib/baseball/redirect-on-unauthorized.ts +// +// Shared guard for Server Component pages that call a `withBaseballAction`- +// wrapped getter directly. Those getters independently re-resolve auth +// (see src/lib/baseball/with-baseball-action.ts) and throw +// BaseballUnauthorizedError when the session has expired in the narrow window +// between the page's own bare `supabase.auth.getUser()` check and the getter +// call. Left uncaught, that raw-throws through the Server Component render +// straight to error.tsx and the error tracker (Sentry/Vercel) instead of the +// honest "please sign in again" redirect. +// +// Deliberately dependency-free (no import of with-baseball-action.ts, which +// pulls in Sentry + the Supabase server client) so every page importing it +// stays cheap to unit-test — callers pass their own `isUnauthorized` +// predicate, typically `(error) => error instanceof BaseballUnauthorizedError`. +// ============================================================================= + +import { redirect } from 'next/navigation'; + +/** + * Runs `action`; if it throws an error `isUnauthorized` recognizes, redirects + * to `/baseball/login` with `loginReturnTo` preserved as `returnTo` instead of + * letting the error propagate. Any other thrown error (a genuine failure for + * an authenticated, authorized caller) is rethrown unchanged so error.tsx + * keeps handling real failures. + */ +export async function redirectOnUnauthorized( + action: () => Promise, + isUnauthorized: (error: unknown) => boolean, + loginReturnTo: string, +): Promise { + try { + return await action(); + } catch (error) { + if (isUnauthorized(error)) { + redirect(`/baseball/login?returnTo=${encodeURIComponent(loginReturnTo)}`); + } + throw error; + } +} diff --git a/src/lib/baseball/with-baseball-action.ts b/src/lib/baseball/with-baseball-action.ts index 0161112f3..869668658 100644 --- a/src/lib/baseball/with-baseball-action.ts +++ b/src/lib/baseball/with-baseball-action.ts @@ -63,6 +63,7 @@ import type { User } from '@supabase/supabase-js'; import { createClient } from '@/lib/supabase/server'; import { logServerException } from '@/lib/server-error-logger'; import { observeActionSoftFailure } from '@/lib/admin/observe-action-result'; +import { maybeCaptureRlsDenial } from '@/lib/admin/rls-denial'; import { getActiveBaseballContext } from '@/lib/baseball/active-context'; import type { ActiveBaseballContext } from '@/lib/baseball/active-context-shared'; import { @@ -483,6 +484,38 @@ export function withBaseballAction( throw error; } + // RLS-denial capture (Helm Bridge capture class #1) — centralized here + // so every one of the 60+ withBaseballAction call sites gets it for + // free, matching golf's per-call-site maybeCaptureRlsDenial coverage + // (golf.ts, teams.ts, event-documents.ts, etc.) without hand-adding a + // call to each action file. This generic wrapper layer never sees the + // specific table a query targeted (that context lives in the action + // body, several calls deep), so `table` falls back to `featureArea` + // and `verb` to the catch-all 'rpc' bucket — good enough to route a + // denial into /admin/errors?source=rls_denial and the rls_denials + // feature-health rollup. `feature` is deliberately omitted: this + // file's `feature` option is a free-form string (not the registry's + // FeatureKey union), so featureForTable's lookup is left to resolve + // it (or fall back to null) rather than passing a mistyped value. + // Action files that know their real table name add a precise + // maybeCaptureRlsDenial call at the swallow site instead (see e.g. + // games.ts saveFullBoxScore, lifting-v11.ts logSetResult). Checked + // against the RAW error, not `normalized` below — normalizing a + // plain PostgrestError-like object into `new Error(String(error))` + // would drop its `.code`. + maybeCaptureRlsDenial( + error && typeof error === 'object' + ? (error as { code?: string | null; message?: string | null }) + : null, + { + table: featureArea, + verb: 'rpc', + action: name, + sport: 'baseball', + userId: observedUserId, + }, + ); + // Unexpected failure: capture the FULL error (stack, pg detail/hint) // through the central logger so it lands in error_logs + admin_events + // Sentry, then rethrow a sanitized error so internals never reach the diff --git a/src/lib/coachhelm/v2/post-round-trigger.ts b/src/lib/coachhelm/v2/post-round-trigger.ts index c350dfeff..e6501c4a6 100644 --- a/src/lib/coachhelm/v2/post-round-trigger.ts +++ b/src/lib/coachhelm/v2/post-round-trigger.ts @@ -18,7 +18,11 @@ */ import type { SupabaseClient } from '@supabase/supabase-js'; import { triggerPlayerInsightsAfterRound } from '@/lib/coachhelm/v2/trigger-insights-bridge'; -import { logServerError } from '@/lib/server-error-logger'; +import { logServerError, logServerEvent } from '@/lib/server-error-logger'; +import { + isExpectedEmptyStateCode, + isExpectedSoftFailureMessage, +} from '@/lib/admin/observe-action-result'; export interface PostRoundTriggerArgs { playerId: string; @@ -53,6 +57,26 @@ type FailureCode = */ const PARTIAL_FAILURE_REASON = 'engine_partial_failure' as const; +/** + * `postRoundTrigger` is a SECOND, independent consumer of the same + * `triggerPlayerInsightsAfterRound` result that `observeActionSoftFailure` + * classifies for the withAdminObserved-wrapped path (see + * src/lib/admin/observe-action-result.ts `severityForSoftFailure`). Both + * consumers must agree on severity for the identical `code` — otherwise a + * routine outcome (no rounds yet, background-context session expiry) is + * simultaneously logged as a handled 'warning'/'info' by one consumer AND as + * a live Sentry exception at 'error' by this one, defeating the whole point + * of the classification. + */ +function classifyEngineFailureSeverity( + message: string, + code: string | null, +): { severity: 'info' | 'warning' | 'error'; skipSentry: boolean } { + if (isExpectedEmptyStateCode(code)) return { severity: 'info', skipSentry: true }; + if (isExpectedSoftFailureMessage(message, code)) return { severity: 'warning', skipSentry: true }; + return { severity: 'error', skipSentry: false }; +} + function sanitizeFailureReason(reason: string): FailureCode { const lower = reason.toLowerCase(); if (lower.includes('timeout') || lower.includes('timed out')) return 'engine_timeout'; @@ -130,12 +154,21 @@ export async function postRoundTrigger( }, 'postRoundTrigger.engineFailure', ); - await logServerError(`postRoundTrigger engine failed: ${reason}`, { + const code = result.code ?? null; + const { severity, skipSentry } = classifyEngineFailureSeverity(reason, code); + const logContext = { action: 'postRoundTrigger.engineFailure', featureArea: 'coachhelm', playerId: args.playerId, + ...(skipSentry ? { skipSentry: true } : {}), + ...(code ? { errorCode: code } : {}), extra: { roundId: args.roundId, triggerReason: args.triggerReason ?? 'round_submitted' }, - }); + }; + if (severity === 'info') { + await logServerEvent(`postRoundTrigger engine failed: ${reason}`, logContext, 'info'); + } else { + await logServerError(`postRoundTrigger engine failed: ${reason}`, logContext, severity); + } return { success: false, error: reason }; } diff --git a/src/lib/coachhelm/v2/trigger-insights-bridge.ts b/src/lib/coachhelm/v2/trigger-insights-bridge.ts index 898fc36c9..418588fd3 100644 --- a/src/lib/coachhelm/v2/trigger-insights-bridge.ts +++ b/src/lib/coachhelm/v2/trigger-insights-bridge.ts @@ -47,7 +47,21 @@ import 'server-only'; type TriggerPlayerInsightsFn = ( playerId: string, -) => Promise<{ success: boolean; insights_created?: number; error?: string; partial?: boolean }>; +) => Promise<{ + success: boolean; + insights_created?: number; + error?: string; + partial?: boolean; + /** + * Stable, non-message-derived classification mirrored from + * triggerPlayerInsightsAfterRoundImpl (src/app/golf/actions/insights.ts) — + * BOTH consumers of this result (the withAdminObserved-wrapped path via + * observeActionSoftFailure, AND postRoundTrigger below in + * post-round-trigger.ts) must classify severity off this same field so + * they don't disagree on the same signal. + */ + code?: 'engine_no_recent_rounds' | 'engine_session_expired'; +}>; let impl: TriggerPlayerInsightsFn | null = null; @@ -69,7 +83,13 @@ export function __registerTriggerPlayerInsightsAfterRound(fn: TriggerPlayerInsig */ export async function triggerPlayerInsightsAfterRound( playerId: string, -): Promise<{ success: boolean; insights_created?: number; error?: string; partial?: boolean }> { +): Promise<{ + success: boolean; + insights_created?: number; + error?: string; + partial?: boolean; + code?: 'engine_no_recent_rounds' | 'engine_session_expired'; +}> { if (!impl) { // Load insights.ts so its registration side effect runs. Cheap after the // first call — subsequent imports hit the module cache. diff --git a/src/lib/notifications/push.ts b/src/lib/notifications/push.ts index 32905d260..0aa9220af 100644 --- a/src/lib/notifications/push.ts +++ b/src/lib/notifications/push.ts @@ -3,6 +3,7 @@ import { createAdminClient } from '@/lib/supabase/admin'; import type { NotificationType, NotificationPreferences } from './types'; import { getUserNotificationPreferences } from './email'; +import { logServerEvent } from '@/lib/server-error-logger'; /** * Check if user wants push notifications for a specific type @@ -167,34 +168,38 @@ export async function sendPushNotification( const payload = generatePushPayload(type, data); - // Send to each device token via Edge Function + // Send to each device token via the Edge Function — invoked through the + // admin client so the service-role auth travels inside the client instead + // of a raw key in a hand-built header (helmv3-service-role-outside-admin). for (const deviceToken of tokens) { try { - const response = await fetch( - `${process.env.NEXT_PUBLIC_SUPABASE_URL}/functions/v1/send-apns-push`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - 'Authorization': `Bearer ${process.env.SUPABASE_SERVICE_ROLE_KEY}`, - }, - body: JSON.stringify({ - deviceToken: deviceToken.token, - platform: deviceToken.platform, - title: payload.title, - body: payload.body, - data: payload.data, - }), - } - ); + const { error: invokeError } = await supabase.functions.invoke('send-apns-push', { + body: { + deviceToken: deviceToken.token, + platform: deviceToken.platform, + title: payload.title, + body: payload.body, + data: payload.data, + }, + }); - if (!response.ok) { - const errorText = await response.text(); + if (invokeError) { // send-apns-push returns { success: false, error, shouldDeactivateToken? } // on 410 (Unregistered) / 400 (BadDeviceToken) — Apple's own "this token // is dead" signal. Parse it so a permanently-dead token stops being // retried on every cron sweep instead of accumulating failed_count - // forever with zero corrective action. + // forever with zero corrective action. On a non-2xx the error's + // `context` is the raw Response (FunctionsHttpError); network-level + // failures have no readable body and fall back to the message. + let errorText = invokeError.message; + const errorContext = (invokeError as { context?: Response }).context; + if (errorContext && typeof errorContext.text === 'function') { + try { + errorText = await errorContext.text(); + } catch { + /* body unreadable — keep the generic error message */ + } + } let shouldDeactivateToken = false; try { const parsed = JSON.parse(errorText) as { shouldDeactivateToken?: boolean }; @@ -212,13 +217,31 @@ export async function sendPushNotification( .single() as { data: { failed_count: number } | null }; // eslint-disable-next-line @typescript-eslint/no-explicit-any - await (supabase as any) + const { error: deactivateError } = await (supabase as any) .from('device_tokens') .update({ failed_count: (currentToken?.failed_count || 0) + 1, ...(shouldDeactivateToken ? { active: false } : {}), }) - .eq('token', deviceToken.token); + .eq('token', deviceToken.token) as { error: { message: string } | null }; + + // Prune confirmation: the row's `active: true` gate on the token + // read above means a deactivated token is never selected again, so + // this fires exactly once per dead token. Logged at info (not + // error/warning) so it surfaces as a routine Sentry message / admin + // feed entry, not an Error issue competing with real incidents. + if (shouldDeactivateToken && !deactivateError) { + await logServerEvent( + `device_tokens pruned: dead APNs token deactivated (user=${userId}, platform=${deviceToken.platform})`, + { + action: 'push.sendPushNotification.pruneDeadToken', + featureArea: 'notifications', + userId, + extra: { tokenPrefix: deviceToken.token.slice(0, 8), platform: deviceToken.platform }, + }, + 'info', + ); + } } else { // Update last_push_at // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/src/test/coachhelm/v2/post-round-trigger.test.ts b/src/test/coachhelm/v2/post-round-trigger.test.ts index a83146f2a..c3fb0859c 100644 --- a/src/test/coachhelm/v2/post-round-trigger.test.ts +++ b/src/test/coachhelm/v2/post-round-trigger.test.ts @@ -9,11 +9,33 @@ vi.mock('@/lib/coachhelm/v2/trigger-insights-bridge', () => ({ triggerPlayerInsightsAfterRound: (...args: unknown[]) => mockTrigger(...args), })); +// Spy on the actual severity/skipSentry passed to the two logging entry +// points. postRoundTrigger is a SECOND independent consumer of the +// triggerPlayerInsightsAfterRound result (alongside the withAdminObserved +// path in insights.ts, verified separately in +// src/lib/admin/__tests__/observe-action-result.test.ts) — these tests pin +// that both consumers classify the same `code` identically instead of this +// one always falling through to default 'error' + live Sentry capture. +const mocks = vi.hoisted(() => ({ + logServerError: vi.fn< + (message: string, context: Record, severity?: string) => Promise + >(async () => {}), + logServerEvent: vi.fn< + (message: string, context: Record, severity?: string) => Promise + >(async () => {}), +})); +vi.mock('@/lib/server-error-logger', () => ({ + logServerError: mocks.logServerError, + logServerEvent: mocks.logServerEvent, +})); + import { postRoundTrigger } from '@/lib/coachhelm/v2/post-round-trigger'; describe('postRoundTrigger', () => { beforeEach(() => { mockTrigger.mockReset(); + mocks.logServerError.mockClear(); + mocks.logServerEvent.mockClear(); }); it('sets coachhelm_analyzed_at and clears failure fields on success', async () => { @@ -91,4 +113,65 @@ describe('postRoundTrigger', () => { await expect(postRoundTrigger(admin as never, { playerId: 'p1', roundId: 'r5' })).resolves.not.toThrow(); }); + + describe('engine-failure severity classification (verdict-softfail-severity)', () => { + it('code: engine_no_recent_rounds logs via logServerEvent at info + skipSentry, never logServerError', async () => { + const admin = createFakeSupabase({ + tables: { golf_rounds: [{ id: 'r6', player_id: 'p1', coachhelm_analyzed_at: null }] }, + }); + mockTrigger.mockResolvedValue({ + success: false, + error: 'No completed rounds in the last 90 days yet — insights will populate after the next round', + code: 'engine_no_recent_rounds', + }); + + await postRoundTrigger(admin as never, { playerId: 'p1', roundId: 'r6' }); + + expect(mocks.logServerEvent).toHaveBeenCalledWith( + expect.stringContaining('postRoundTrigger engine failed'), + expect.objectContaining({ skipSentry: true, errorCode: 'engine_no_recent_rounds' }), + 'info', + ); + expect(mocks.logServerError).not.toHaveBeenCalledWith( + expect.stringContaining('postRoundTrigger engine failed'), + expect.anything(), + expect.anything(), + ); + }); + + it('code: engine_session_expired logs via logServerError at warning + skipSentry (handled, not a live Sentry exception)', async () => { + const admin = createFakeSupabase({ + tables: { golf_rounds: [{ id: 'r7', player_id: 'p1', coachhelm_analyzed_at: null }] }, + }); + mockTrigger.mockResolvedValue({ + success: false, + error: 'Player analysis failed (likely session expired in background context)', + code: 'engine_session_expired', + }); + + await postRoundTrigger(admin as never, { playerId: 'p1', roundId: 'r7' }); + + expect(mocks.logServerError).toHaveBeenCalledWith( + expect.stringContaining('postRoundTrigger engine failed'), + expect.objectContaining({ skipSentry: true, errorCode: 'engine_session_expired' }), + 'warning', + ); + }); + + it('regression guard: an unrecognized code/message still logs at default error severity with no skipSentry', async () => { + const admin = createFakeSupabase({ + tables: { golf_rounds: [{ id: 'r8', player_id: 'p1', coachhelm_analyzed_at: null }] }, + }); + mockTrigger.mockResolvedValue({ success: false, error: 'team disabled coachhelm' }); + + await postRoundTrigger(admin as never, { playerId: 'p1', roundId: 'r8' }); + + const call = mocks.logServerError.mock.calls.find(([msg]) => + String(msg).startsWith('postRoundTrigger engine failed'), + ); + expect(call).toBeTruthy(); + expect(call?.[2]).toBe('error'); + expect((call?.[1] as Record | undefined)?.skipSentry).toBeUndefined(); + }); + }); }); diff --git a/src/test/lib/notifications/push.test.ts b/src/test/lib/notifications/push.test.ts index e505de187..04ca505ef 100644 --- a/src/test/lib/notifications/push.test.ts +++ b/src/test/lib/notifications/push.test.ts @@ -7,6 +7,12 @@ * only ever read via `.text()`), so a permanently-dead token accumulated * failed_count forever without ever being deactivated — retried on every * scheduled push sweep indefinitely. + * + * Also covers the info-severity prune log emitted when a dead token is + * deactivated (ROOT CAUSE 1 follow-up): the send path is generic, so every + * caller (message pushes, the CoachHelm v3 dispatcher, the roster-sweep + * cron's downstream insight notifications, etc.) gets the prune + the log + * for free instead of each caller reimplementing it. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -17,14 +23,41 @@ vi.mock('@/lib/notifications/email', () => ({ getUserNotificationPreferences: (userId: string) => getUserNotificationPreferencesMock(userId), })); +const logServerEventMock = vi.fn(async (..._args: unknown[]) => {}); +vi.mock('@/lib/server-error-logger', () => ({ + logServerEvent: (...args: unknown[]) => logServerEventMock(...args), +})); + type TokenRow = { token: string; platform: string }; +/** + * Mirror of `supabase.functions.invoke` failure semantics: non-2xx responses + * surface as an error whose `context` is the raw Response (FunctionsHttpError + * shape — push.ts duck-types `context.text()` rather than instanceof so the + * mock only needs the shape). + */ +function makeFunctionsInvokeMock(failureResponse: Response | null) { + return vi.fn(async () => + failureResponse + ? { + data: null, + error: { + message: 'Edge Function returned a non-2xx status code', + context: failureResponse, + }, + } + : { data: { success: true }, error: null }, + ); +} + function makeDeviceTokensFromMock(opts: { tokens: TokenRow[]; currentFailedCount?: number; updateSpy: (payload: Record) => void; + /** Simulate the deactivate UPDATE itself failing (e.g. RLS/connection). */ + updateError?: { message: string }; }) { - const { tokens, currentFailedCount = 0, updateSpy } = opts; + const { tokens, currentFailedCount = 0, updateSpy, updateError = null } = opts; return vi.fn((table: string) => { if (table !== 'device_tokens') throw new Error(`unexpected table ${table}`); let mode: 'select-list' | 'update' = 'select-list'; @@ -47,7 +80,8 @@ function makeDeviceTokensFromMock(opts: { eq: vi.fn(() => builder), single: vi.fn(() => Promise.resolve({ data: { failed_count: currentFailedCount } })), then: (resolve, reject) => { - const result = mode === 'update' ? { data: null, error: null } : { data: tokens, error: null }; + const result = + mode === 'update' ? { data: null, error: updateError } : { data: tokens, error: null }; return Promise.resolve(result).then(resolve, reject); }, }; @@ -56,16 +90,14 @@ function makeDeviceTokensFromMock(opts: { } describe('sendPushNotification — APNs failure-response parsing', () => { - const originalFetch = global.fetch; - beforeEach(() => { getUserNotificationPreferencesMock.mockClear(); + logServerEventMock.mockClear(); vi.stubEnv('NEXT_PUBLIC_SUPABASE_URL', 'https://xyz.supabase.co'); vi.stubEnv('SUPABASE_SERVICE_ROLE_KEY', 'service-role-key'); }); afterEach(() => { - global.fetch = originalFetch; vi.unstubAllEnvs(); vi.resetModules(); }); @@ -77,11 +109,7 @@ describe('sendPushNotification — APNs failure-response parsing', () => { currentFailedCount: 130, updateSpy, }); - vi.doMock('@/lib/supabase/admin', () => ({ - createAdminClient: () => ({ from: fromMock }), - })); - - global.fetch = vi.fn(async () => + const invokeMock = makeFunctionsInvokeMock( new Response( JSON.stringify({ success: false, @@ -90,16 +118,37 @@ describe('sendPushNotification — APNs failure-response parsing', () => { }), { status: 410 }, ), - ) as unknown as typeof fetch; + ); + vi.doMock('@/lib/supabase/admin', () => ({ + createAdminClient: () => ({ from: fromMock, functions: { invoke: invokeMock } }), + })); const { sendPushNotification } = await import('@/lib/notifications/push'); const result = await sendPushNotification('task_reminder', 'user-1', { taskTitle: 'Log rounds' }); expect(result.success).toBe(true); + expect(invokeMock).toHaveBeenCalledWith( + 'send-apns-push', + expect.objectContaining({ + body: expect.objectContaining({ deviceToken: 'dead-token-1234567890', platform: 'ios' }), + }), + ); expect(updateSpy).toHaveBeenCalledTimes(1); expect(updateSpy).toHaveBeenCalledWith( expect.objectContaining({ active: false, failed_count: 131 }), ); + + // Prune is logged exactly once, at info severity, and never hits Sentry + // as an Error issue (message/context first, severity last). + expect(logServerEventMock).toHaveBeenCalledTimes(1); + const [message, context, severity] = logServerEventMock.mock.calls[0]!; + expect(message).toContain('pruned'); + expect(message).toContain('user-1'); + expect(context).toMatchObject({ + action: 'push.sendPushNotification.pruneDeadToken', + userId: 'user-1', + }); + expect(severity).toBe('info'); }); it('does NOT deactivate the token on a generic/transient failure (no shouldDeactivateToken flag)', async () => { @@ -109,16 +158,15 @@ describe('sendPushNotification — APNs failure-response parsing', () => { currentFailedCount: 2, updateSpy, }); - vi.doMock('@/lib/supabase/admin', () => ({ - createAdminClient: () => ({ from: fromMock }), - })); - - global.fetch = vi.fn(async () => + const invokeMock = makeFunctionsInvokeMock( new Response( JSON.stringify({ success: false, error: 'APNs error 502: upstream timeout' }), { status: 502 }, ), - ) as unknown as typeof fetch; + ); + vi.doMock('@/lib/supabase/admin', () => ({ + createAdminClient: () => ({ from: fromMock, functions: { invoke: invokeMock } }), + })); const { sendPushNotification } = await import('@/lib/notifications/push'); const result = await sendPushNotification('task_reminder', 'user-1', { taskTitle: 'Log rounds' }); @@ -128,6 +176,7 @@ describe('sendPushNotification — APNs failure-response parsing', () => { const payload = updateSpy.mock.calls[0]?.[0] as Record; expect(payload).not.toHaveProperty('active'); expect(payload.failed_count).toBe(3); + expect(logServerEventMock).not.toHaveBeenCalled(); }); it('does not throw and does not deactivate when the failure body is not JSON', async () => { @@ -137,12 +186,11 @@ describe('sendPushNotification — APNs failure-response parsing', () => { currentFailedCount: 0, updateSpy, }); + const invokeMock = makeFunctionsInvokeMock(new Response('Internal Server Error', { status: 500 })); vi.doMock('@/lib/supabase/admin', () => ({ - createAdminClient: () => ({ from: fromMock }), + createAdminClient: () => ({ from: fromMock, functions: { invoke: invokeMock } }), })); - global.fetch = vi.fn(async () => new Response('Internal Server Error', { status: 500 })) as unknown as typeof fetch; - const { sendPushNotification } = await import('@/lib/notifications/push'); const result = await sendPushNotification('task_reminder', 'user-1', { taskTitle: 'Log rounds' }); @@ -150,5 +198,40 @@ describe('sendPushNotification — APNs failure-response parsing', () => { const payload = updateSpy.mock.calls[0]?.[0] as Record; expect(payload).not.toHaveProperty('active'); expect(payload.failed_count).toBe(1); + expect(logServerEventMock).not.toHaveBeenCalled(); + }); + + it('does not log the prune when the deactivating UPDATE itself errors', async () => { + const updateSpy = vi.fn(); + const fromMock = makeDeviceTokensFromMock({ + tokens: [{ token: 'dead-token-but-update-fails', platform: 'ios' }], + currentFailedCount: 5, + updateSpy, + updateError: { message: 'connection reset' }, + }); + const invokeMock = makeFunctionsInvokeMock( + new Response( + JSON.stringify({ + success: false, + error: 'APNs error 410: Unregistered', + shouldDeactivateToken: true, + }), + { status: 410 }, + ), + ); + vi.doMock('@/lib/supabase/admin', () => ({ + createAdminClient: () => ({ from: fromMock, functions: { invoke: invokeMock } }), + })); + + const { sendPushNotification } = await import('@/lib/notifications/push'); + const result = await sendPushNotification('task_reminder', 'user-1', { taskTitle: 'Log rounds' }); + + expect(result.success).toBe(true); + // The row is still asked to deactivate (the caller-side intent is + // correct) — only the confirmation log is gated on the write succeeding, + // so a token that silently fails to deactivate doesn't falsely claim to + // be pruned in the admin feed / Sentry. + expect(updateSpy).toHaveBeenCalledWith(expect.objectContaining({ active: false })); + expect(logServerEventMock).not.toHaveBeenCalled(); }); });