-
Notifications
You must be signed in to change notification settings - Fork 0
Error sweep: APNs token pruning, honest severities, unauth redirects, baseball RLS-denial parity #798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error sweep: APNs token pruning, honest severities, unauth redirects, baseball RLS-denial parity #798
Changes from 1 commit
fc48386
6ebf856
6159650
285508a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, | ||
| })); | ||
|
Comment on lines
+43
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Extract the duplicated This exact ~12-line class + ♻️ Proposed shared helper// src/test/baseball/fake-unauthorized-error.ts
export class FakeBaseballUnauthorizedError extends Error {
readonly status = 401;
constructor(message = 'You must be signed in.') {
super(message);
this.name = 'BaseballUnauthorizedError';
}
}+import { FakeBaseballUnauthorizedError } from '`@/test/baseball/fake-unauthorized-error`';
-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,
}));🤖 Prompt for AI Agents |
||
|
|
||
| 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(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| 'use client'; | ||
|
|
||
| import { RouteErrorBoundary } from '@/components/errors'; | ||
|
|
||
| export default function Error({ | ||
| error, | ||
| reset, | ||
| }: { | ||
| error: Error & { digest?: string }; | ||
| reset: () => void; | ||
| }) { | ||
| return ( | ||
| <RouteErrorBoundary | ||
| error={error} | ||
| reset={reset} | ||
| route="/baseball/dashboard/operations" | ||
| component="OperationsPage" | ||
| title="Couldn't load Operations" | ||
| message="We couldn't load the team logistics hub. Please try again." | ||
| homePath="/baseball/dashboard/roster" | ||
| /> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| 'use client'; | ||
|
|
||
| import { RouteErrorBoundary } from '@/components/errors'; | ||
|
|
||
| export default function Error({ | ||
| error, | ||
| reset, | ||
| }: { | ||
| error: Error & { digest?: string }; | ||
| reset: () => void; | ||
| }) { | ||
| return ( | ||
| <RouteErrorBoundary | ||
| error={error} | ||
| reset={reset} | ||
| route="/baseball/dashboard/performance/builder" | ||
| component="LiftBuilderPage" | ||
| title="Couldn't load the Lift Builder" | ||
| message="We couldn't load the lift planning tools. Please try again." | ||
| homePath="/baseball/dashboard/performance" | ||
| /> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<ReturnType<typeof getScoutPacketPreview>>; | ||
| 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; | ||
| } | ||
|
Comment on lines
+37
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate the target page and shared helper.
git ls-files 'src/app/baseball/**/preview/page.tsx' 'src/lib/baseball/**' | sed -n '1,200p'
printf '\n--- helper references ---\n'
rg -n "redirectOnUnauthorized|BaseballUnauthorizedError|getScoutPacketPreview|withBaseballAction" src/app/baseball src/lib/baseball -g '!**/node_modules/**' | sed -n '1,240p'Repository: njrini99-code/helmv3 Length of output: 32977 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Read the target page and the helper implementation/call sites.
sed -n '1,220p' src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx
printf '\n--- helper ---\n'
sed -n '1,220p' src/lib/baseball/redirect-on-unauthorized.ts
printf '\n--- similar pages ---\n'
rg -n "redirectOnUnauthorized\(" src/app/baseball/(dashboard)/dashboard -g 'page.tsx' -g '!**/node_modules/**' | sed -n '1,240p'Repository: njrini99-code/helmv3 Length of output: 211 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- target file ---'
sed -n '1,220p' 'src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx'
echo
echo '--- helper file candidates ---'
git ls-files 'src/lib/baseball/*redirect*' 'src/lib/baseball/**/redirect*' | sed -n '1,120p'
echo
echo '--- helper definition/calls ---'
rg -n "redirectOnUnauthorized|BaseballUnauthorizedError" src/lib/baseball src/app/baseball -g '!**/node_modules/**' | sed -n '1,240p'Repository: njrini99-code/helmv3 Length of output: 31817 Use 🤖 Prompt for AI Agents |
||
|
|
||
| return ( | ||
| <div className="min-h-dvh bg-cream-100"> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: njrini99-code/helmv3
Length of output: 50376
🏁 Script executed:
Repository: njrini99-code/helmv3
Length of output: 50377
Use the shared Supabase fixture here
createClientmock atsrc/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx:27-31withcreateFakeSupabasefromsrc/test/fixtures/fake-supabase.ts:1-20; this page test only needsauth.getUser(), and the shared fixture is the repo convention.src/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx:84-88;toBeTruthy()on the returned element is too weak—assert the rendered output or passed props instead.🤖 Prompt for AI Agents
Source: Path instructions