Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/app/api/cron/coachhelm-roster-sweep/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
*/
import { NextResponse, type NextRequest } from 'next/server';
import { createAdminClient } from '@/lib/supabase/admin';
// Side-effect-only: guarantees insights.ts's module-scope registration
// (__registerTriggerPlayerInsightsAfterRound) has run at module-init time —
// the bridge no longer lazily imports insights.ts itself (that dynamic
// back-edge was the cold-start TDZ cycle; see trigger-insights-bridge.ts).
import '@/app/golf/actions/insights';
import { triggerPlayerInsightsAfterRound } from '@/lib/coachhelm/v2/trigger-insights-bridge';
import { logServerError } from '@/lib/server-error-logger';
import { requireCronAuth } from '@/lib/cron/auth';
Expand Down
18 changes: 16 additions & 2 deletions src/app/golf/(dashboard)/dashboard/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,20 @@ export const dynamic = 'force-dynamic';

const VALID_RANGES = new Set(['7d', '30d', '90d', 'season', 'all']);

/**
* A session can pass the top-of-page check yet expire before the data fetch
* re-validates it (auth token refresh race — observed live as repeated
* 'Not authenticated' digests on /golf/dashboard). Send that narrow case to
* the login screen; anything else still surfaces to the route error boundary
* (P002/P426 — real outages must never be swallowed).
*/
function redirectToLoginOnExpiredSession(error: unknown): never {
if (error instanceof Error && error.message === 'Not authenticated') {
redirect('/golf/login?returnTo=/golf/dashboard');
}
throw error;
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
}
Comment thread
qodo-code-review[bot] marked this conversation as resolved.

/**
* Renders the coach dashboard inside the `.fairway-ds` scope on `bg-canvas`.
* Fairway is the only tree (Wave W1) — the legacy CoachDashboard fork has
Expand Down Expand Up @@ -126,7 +140,7 @@ export default async function GolfDashboardPage({
const [payload, joinRequestsResult] = await Promise.all([
getCachedCoachDashboardData(coach.id, userId, teamId, dateRange),
getTeamJoinRequests(),
]);
]).catch(redirectToLoginOnExpiredSession);
const joinRequests: JoinRequestData[] =
joinRequestsResult.success && joinRequestsResult.data ? joinRequestsResult.data : [];

Expand Down Expand Up @@ -201,7 +215,7 @@ export default async function GolfDashboardPage({
const [payload, hubData] = await Promise.all([
getCachedPlayerDashboardData(player.id, userId, teamId),
teamId ? getPlayerHubSummaryData(teamId, player.id) : Promise.resolve(null),
]);
]).catch(redirectToLoginOnExpiredSession);
const nameParts = `${player.first_name} ${player.last_name}`.split(' ');

const data: PlayerDashboardData = {
Expand Down
15 changes: 15 additions & 0 deletions src/instrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,21 @@ const sharedIgnoreErrors = [
// suppressed; every other AuthApiError now reaches Sentry.
/AuthApiError: Invalid Refresh Token/,
/Refresh Token Not Found/,
// Baseball expected control-flow throws. withBaseballAction already
// classifies these as handled/expected (admin_events + Sentry warning with
// skipSentry) and then RE-RAISES so callers can branch — but the re-raise
// escapes the server-action boundary, where Next's own console.error +
// onRequestError capture it a second time as an unhandled Sentry Error
// (observed live: a logged-out tab's 60 s NotificationBell poll produced
// "BaseballUnauthorizedError: You must be signed in"). These stay fully
// visible in admin_events / Helm Bridge; only the duplicate Sentry Error
// is suppressed.
'BaseballUnauthorizedError',
'BaseballNoActiveTeamError',
'BaseballCapabilityError',
'BaseballDisabledSourceError',
'BaseballDemoReadOnlyError',
'PlayerAccessError',
];

const scrubPii: Sentry.NodeOptions['beforeSend'] = (event) => {
Expand Down
28 changes: 20 additions & 8 deletions src/lib/coachhelm/v2/mining/pattern-miner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -992,14 +992,26 @@ export class PatternMiner {
const playerIds = [...new Set(patterns.map((p) => p.playerId).filter(Boolean))];
if (ids.length > 0 && playerIds.length > 0) {
const idList = `(${ids.map((id) => `"${id}"`).join(',')})`;
const { error: supersedeError } = await fromUntyped(supabase, 'golf_patterns_v2')
.update({ is_active: false, updated_at: new Date().toISOString() })
.in('player_id', playerIds)
.eq('is_active', true)
.not('id', 'in', idList)
// Keep coach-curated patterns visible; only retire auto-detected ones
// (NULL or non-preserved lifecycle_state).
.or('lifecycle_state.is.null,lifecycle_state.not.in.(confirmed,addressed,resolved,dismissed)');
// Multi-row UPDATE racing a concurrent mine's upserts/supersede over the
// same players can deadlock (observed live: 40P01 during the roster-sweep
// cron, round-submit trigger and cron batch mining the same team
// concurrently). Postgres aborts one victim wholesale and the supersede
// is idempotent (deterministic ids, converges on re-run), so a short
// retry absorbs it instead of surfacing a transient as an error.
let supersedeError: { code?: string } | null = null;
for (let attempt = 0; attempt < 3; attempt++) {
if (attempt > 0) await new Promise((resolve) => setTimeout(resolve, 200 * attempt));
const { error } = await fromUntyped(supabase, 'golf_patterns_v2')
.update({ is_active: false, updated_at: new Date().toISOString() })
.in('player_id', playerIds)
.eq('is_active', true)
.not('id', 'in', idList)
// Keep coach-curated patterns visible; only retire auto-detected ones
// (NULL or non-preserved lifecycle_state).
.or('lifecycle_state.is.null,lifecycle_state.not.in.(confirmed,addressed,resolved,dismissed)');
supersedeError = error;
Comment on lines +1009 to +1017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Supersede Failure Leaves Stale Patterns

When the new retry loop exhausts or sees any non-40P01 error, savePatterns only logs and returns after the earlier upserts have already committed. That leaves both the newly upserted active patterns and the old patterns that this update was supposed to retire visible until a later mine happens to clean them up.

Rule Used: No DELETE-then-INSERT in any save/submit/sync path... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/coachhelm/v2/mining/pattern-miner.ts
Line: 1004-1012

Comment:
**Supersede Failure Leaves Stale Patterns**

When the new retry loop exhausts or sees any non-`40P01` error, `savePatterns` only logs and returns after the earlier upserts have already committed. That leaves both the newly upserted active patterns and the old patterns that this update was supposed to retire visible until a later mine happens to clean them up.

**Rule Used:** No DELETE-then-INSERT in any save/submit/sync path... ([source](.greptile))

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

if (!error || error.code !== '40P01') break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Deadlock Retry Depends On Code Shape

This only retries when the returned error has code === '40P01'. If the Supabase/PostgREST layer surfaces the observed deadlock as a thrown exception or a wrapped error without that exact field, the loop breaks after the first attempt and the live deadlock path still logs a failed supersede without retrying.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/coachhelm/v2/mining/pattern-miner.ts
Line: 1013

Comment:
**Deadlock Retry Depends On Code Shape**

This only retries when the returned error has `code === '40P01'`. If the Supabase/PostgREST layer surfaces the observed deadlock as a thrown exception or a wrapped error without that exact field, the loop breaks after the first attempt and the live deadlock path still logs a failed supersede without retrying.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (supersedeError) {
await logServerError('pattern-miner.savePatterns supersede stale', {
action: 'pattern-miner.savePatterns',
Expand Down
6 changes: 6 additions & 0 deletions src/lib/coachhelm/v2/post-round-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@
* NOT a replacement for triggerPlayerInsightsAfterRound — wraps it.
*/
import type { SupabaseClient } from '@supabase/supabase-js';
// Side-effect-only: guarantees insights.ts's module-scope registration
// (__registerTriggerPlayerInsightsAfterRound) has run before this module's
// own top-level finishes — at synchronous module-init time, so there is no
// async gap for a concurrent request on the same warm instance to race
// against (the cold-start TDZ crash the bridge header documents).
import '@/app/golf/actions/insights';
import { triggerPlayerInsightsAfterRound } from '@/lib/coachhelm/v2/trigger-insights-bridge';
import { logServerError, logServerEvent } from '@/lib/server-error-logger';
import {
Expand Down
29 changes: 17 additions & 12 deletions src/lib/coachhelm/v2/trigger-insights-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,21 @@ import 'server-only';
*
* Wiring: insights.ts calls `__registerTriggerPlayerInsightsAfterRound(...)`
* at its own module scope (a side effect, not an export) to hand this
* bridge a reference to the withAdminObserved-wrapped impl.
* `triggerPlayerInsightsAfterRound` below lazily triggers that registration
* (via a dynamic import of insights.ts) the first time it's called in a
* given process, then delegates to the registered function on every call —
* same object, same behavior, just never exported from a 'use server'
* module. The one legitimate CLIENT-reachable path (a coach manually
* bridge a reference to the withAdminObserved-wrapped impl. Each legitimate
* caller (post-round-trigger.ts, the roster-sweep cron route) carries a
* side-effect `import '@/app/golf/actions/insights'` so that registration
* has ALREADY run, synchronously, by the time the caller's own module
* finishes initializing. This used to be a lazy `await import()` inside
* `triggerPlayerInsightsAfterRound` below — but that made this file the
* dynamic back-edge of a value-level import cycle with insights.ts
* (insights.ts statically imports the register function above), and on a
* cold Fluid Compute instance two concurrent first-touches of the pair
* (a background after() trigger racing a dashboard server action) could
* observe insights.ts mid-evaluation — the intermittent cold-start
* "Cannot access 'a' before initialization" TDZ crash in generateAlerts.
* `triggerPlayerInsightsAfterRound` now only delegates to the registered
* function — same object, same behavior, just never exported from a
* 'use server' module. The one legitimate CLIENT-reachable path (a coach manually
* refreshing a player's analysis) is unaffected — it goes through the
* already-authed sibling action `refreshPlayerAnalysisAsCoach`, which is
* still exported from insights.ts and calls the private impl directly
Expand Down Expand Up @@ -90,15 +99,11 @@ export async function triggerPlayerInsightsAfterRound(
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.
await import('@/app/golf/actions/insights');
}
if (!impl) {
throw new Error(
'[trigger-insights-bridge] triggerPlayerInsightsAfterRound not registered — ' +
'insights.ts failed to load or its registration call was removed',
"the caller is missing its side-effect import of '@/app/golf/actions/insights' " +
'(see the wiring note in this file header), or the registration call was removed',
);
}
return impl(playerId);
Expand Down
6 changes: 6 additions & 0 deletions src/test/coachhelm/v2/post-round-trigger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ vi.mock('@/lib/coachhelm/v2/trigger-insights-bridge', () => ({
triggerPlayerInsightsAfterRound: (...args: unknown[]) => mockTrigger(...args),
}));

// post-round-trigger.ts carries a side-effect `import '@/app/golf/actions/insights'`
// (registers the bridge impl at module-init time — the fix for the cold-start
// TDZ cycle). The bridge is fully mocked above, so the registration side effect
// is irrelevant here — stub the whole 4,400-line 'use server' module out.
vi.mock('@/app/golf/actions/insights', () => ({}));

// 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
Expand Down
Loading