fix: feature-flow sweep — 47 confirmed broken/stranded flows fixed across golf, baseball, and Bridge#802
Merged
Merged
Conversation
…ross golf, baseball, and Bridge An exhaustive per-cluster flow trace (every control -> action -> write -> revalidate -> consuming surface) found 148 candidate defects; 47 survived adversarial confirmation (28 P0 / 19 P1) and are fixed here by 17 verified packets plus lead hand-repairs. Highest impact: - Baseball pending joiners un-stranded: approve/reject flow built into the roster Status board (require_coach_approval defaults true on every prod team; joiners previously vanished into 'pending' with no UI anywhere). - Built-but-dead controls wired: player-notes edit/delete, lineup Share + a saved-lineups panel (load/edit/delete), remove-player + add-existing- player + jersey/position edits, class-schedule UI. - Roster/leaderboard stats read-model now merges current-season box-score stats over the legacy CSV aggregates (players stop reading 'Needs Data'). - Silent data loss closed: travel link-to-event edit (schema dropped event_id), recurring events now invite attendees + notify (create/edit/ delete, via after()), imports honesty (deprecated-table writes surfaced), task-reminder cron now actually matches the reminders users set. - Messaging: unread badges can finally clear (participant-checked SECURITY DEFINER RPC replaces an RLS-blocked update), broadcast dedupe no longer swallows new announcements into old threads by title. - Qualifiers: full lifecycle now reachable — edit details, auto in_progress on first round, auto completed when every entrant is done (incl. the single-entrant num_rounds=1 fall-through) plus a view-time reconcile for deadline-passed qualifiers; num_rounds caps submissions. - Notifications: quiet-mode preference actually enforced, CoachHelm pushes route through their real preference key, in-app receipts surfaced. - Bridge work log reads PR lifecycle from fields the GitHub API returns. - Practice-block visibility enforced by RLS (staff_only/restricted no longer readable or rendered player-side). DB: four replay-safe migrations (settings-audit reconcile + capability- scoped INSERT policy, decision-log reconcile, practice-blocks visibility RLS, mark_golf_messages_read RPC with anon revoked) applied to prod and verified via pg_policy/information_schema/proacl. Contracts updated with the new truth: feature-registry qualifiers manifest (+2, counts 419->421), stat-layer manifest grandfathers the roster aggregates bridging shim, golf observability wraps reconcileQualifierStatus. Residual (tracked, not closed): savePartialRound can still collide on two same-course/same-day in-progress rounds — full fix is client-generated round ids. Gates: tsc, eslint --max-warnings 0, FULL vitest suite (19,641 tests, 1,949 files), production build — all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Too many files changed for review. ( Bypass the limit by tagging |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Updates to Preview Branch (fix/feature-flows) ↗︎
Tasks are run on every commit but only new migration files are pushed.
❌ Branch Error • Fri, 10 Jul 2026 20:02:30 UTC View logs for this Workflow Run ↗︎. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThis PR expands baseball and golf workflows with authorization changes, roster and academic interfaces, canonical import persistence, qualifier lifecycle management, messaging notifications, recurring-event fanout, focus-area controls, review notes, and database reconciliation migrations. ChangesBaseball workflows
Golf workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 🔧 ESLint
src/app/actions/messages.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/app/baseball/(dashboard)/dashboard/roster/RosterFairway.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/components/fairway/pages/qualifiers/FairwayQualifierDetail.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| open | ||
| onClose={() => setClassesStudent(null)} | ||
| playerId={classesStudent.player_id} | ||
| teamId={selectedTeamId ?? ''} |
PR Summary by QodoFeature-flow sweep: 47 fixes for broken/stranded flows in golf, baseball, Bridge
AI Description
Diagram
High-Level Assessment
Files changed (95)
|
coderabbitai
Bot
added
database
Code Review by Qodo
Context used✅ Compliance rules (platform):
98 rules 1. any used in block filter
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // (20260710150000_baseball_practice_blocks_visibility_rls.sql). | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const visibleBlocks = (p.blocks ?? []).filter((b: any) => b.visibility !== 'staff_only'); | ||
| const visibleBlocks = (p.blocks ?? []).filter((b: any) => b.visibility == null || b.visibility === 'player_visible'); |
There was a problem hiding this comment.
2. any used in block filter 📘 Rule violation ⚙ Maintainability
The updated practice visibility filter keeps an explicit any annotation, violating the no-any TypeScript requirement. This weakens type safety around visibility enforcement and can hide future logic/type regressions.
Agent Prompt
## Issue description
Changed code introduces/retains explicit `any` in TypeScript in modified lines.
## Issue Context
Compliance requires eliminating `any` annotations/assertions in changed TS/TSX. These instances are in production action code paths.
## Fix Focus Areas
- src/app/baseball/actions/practice.ts[763-766]
- src/app/actions/messages.ts[401-413]
- src/app/golf/actions/recurring-events.ts[682-688]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,40 @@ | |||
| 'use server'; | |||
There was a problem hiding this comment.
3. use server file outside actions 📘 Rule violation ⌂ Architecture
A new server action module ('use server') was added under src/app/baseball/actions/ instead of
src/app/actions/. This violates the required server-action placement convention and makes action
discovery/enforcement inconsistent.
Agent Prompt
## Issue description
Server action files must live under `src/app/actions/`, but this PR adds a `'use server'` module under `src/app/baseball/actions/`.
## Issue Context
The module is clearly a server action file (top-level `'use server'` directive) and exports an action wrapper.
## Fix Focus Areas
- src/app/baseball/actions/practice-planner-capability.ts[1-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| */ | ||
| export const getCanManagePractice = withBaseballAction( | ||
| 'getCanManagePractice', | ||
| { feature: 'baseball_practice', featureArea: 'baseball-practice' }, |
There was a problem hiding this comment.
4. baseball_practice missing registry mapping 📘 Rule violation ⚙ Maintainability
The new/modified action metadata references the feature identifier baseball_practice, but there is no corresponding entry visible in memory/registry.yml. This likely violates the requirement to keep feature registry mappings in sync with feature identifiers used in code.
Agent Prompt
## Issue description
A feature identifier referenced in changed code (`baseball_practice`) does not appear to have a mapping (or explicit gap marker) in `memory/registry.yml`.
## Issue Context
The action metadata is used for feature-aware observability/governance; the registry should include a mapping or an explicit awareness-gap marker per convention.
## Fix Focus Areas
- src/app/baseball/actions/practice-planner-capability.ts[33-36]
- memory/registry.yml[110-147]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+66
| const { data: otherParticipants } = await supabase | ||
| .from('golf_conversation_participants') | ||
| .select('user_id') | ||
| .eq('conversation_id', conversationId) | ||
| .neq('user_id', senderId); | ||
|
|
||
| if (!otherParticipants || otherParticipants.length === 0) return; | ||
|
|
||
| const recipientUserIds = otherParticipants.map(p => p.user_id); | ||
|
|
||
| // Batch every lookup into ONE round-trip of parallel queries: sender name | ||
| // (coach → player → email fallback) and all recipient emails via a single | ||
| // .in() instead of sequential per-recipient fetches. | ||
| const [ | ||
| { data: senderCoach }, | ||
| { data: senderPlayer }, | ||
| { data: senderUser }, | ||
| { data: recipientProfiles }, | ||
| ] = await Promise.all([ | ||
| supabase.from('golf_coaches').select('full_name').eq('user_id', senderId).maybeSingle(), | ||
| supabase.from('golf_players').select('first_name, last_name').eq('user_id', senderId).maybeSingle(), | ||
| supabase.from('users').select('email').eq('id', senderId).maybeSingle(), | ||
| supabase.from('users').select('id, email').in('id', recipientUserIds), | ||
| ]); | ||
|
|
||
| const senderName = senderCoach?.full_name | ||
| || (senderPlayer ? `${senderPlayer.first_name || ''} ${senderPlayer.last_name || ''}`.trim() : '') | ||
| || senderUser?.email | ||
| || 'Someone'; | ||
|
|
||
| if (!recipientProfiles) return; | ||
|
|
There was a problem hiding this comment.
5. Message fanout recipient lookup fails 🐞 Bug ≡ Correctness
notifyGolfMessageRecipients excludes the sender from recipientUserIds, then attempts to SELECT recipient rows from public.users using the request-scoped Supabase client, but users RLS only permits selecting your own row—so recipientProfiles will be empty and email/push/in-app notifications won’t be sent to recipients.
Agent Prompt
### Issue description
`notifyGolfMessageRecipients()` builds `recipientUserIds` as “all other participants” and then calls `supabase.from('users').select('id, email').in('id', recipientUserIds)`. With current RLS (`users_select_own`), the sender cannot read any of those rows (they are *explicitly not* the sender), so the function returns early and no notifications are sent.
### Issue Context
- The server `createClient()` uses the anon key + user cookies, so RLS applies.
- `users_select_own` restricts SELECT to `auth.uid() = id`.
- `recipientUserIds` explicitly excludes `senderId`, guaranteeing the RLS-visible set is empty.
### Fix Focus Areas
- src/lib/notifications/golf-message-fanout.ts[35-105]
- supabase/migrations/20260527000000_prod_public_baseline.sql[20153-20164]
- src/lib/supabase/server.ts[9-24]
### Implementation notes
- Use `createAdminClient()` (or a purpose-built RPC) to resolve recipient emails (and optionally ids) for notification fanout.
- For push + in-app notifications, you don’t need `users` rows at all—use `recipientUserIds` directly.
- Keep best-effort semantics (do not fail the message send on notification failures), but log per-channel failures with enough context (conversationId, recipient count).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+615
to
+651
| // A broadcast with this title exists, but for a DIFFERENT audience | ||
| // than the one just selected. Reuse the oldest such thread (rather | ||
| // than fragmenting into yet another same-titled conversation) but | ||
| // reconcile membership first: add anyone newly selected who isn't | ||
| // already a participant, so the message the coach is about to send | ||
| // actually reaches every recipient they just chose. Never remove | ||
| // existing participants here — golf messaging never does destructive | ||
| // participant writes. | ||
| const reuseTarget = existingConvs[0]; | ||
| if (reuseTarget) { | ||
| const { data: existingParticipants } = await supabase | ||
| .from('golf_conversation_participants') | ||
| .select('user_id') | ||
| .eq('conversation_id', reuseTarget.id); | ||
|
|
||
| const existingIds = new Set((existingParticipants ?? []).map((p) => p.user_id)); | ||
| const missingIds = desiredParticipantIds.filter((id) => !existingIds.has(id)); | ||
|
|
||
| if (missingIds.length > 0) { | ||
| const { error: syncError } = await supabase | ||
| .from('golf_conversation_participants') | ||
| .insert( | ||
| missingIds.map((userId) => ({ | ||
| conversation_id: reuseTarget.id, | ||
| user_id: userId, | ||
| joined_at: new Date().toISOString(), | ||
| })), | ||
| ); | ||
|
|
||
| if (syncError) { | ||
| await logServerError(`[Broadcast] Failed to sync participants on reuse: ${syncError instanceof Error ? syncError.message : String(syncError)}`, { action: 'messages.createGolfTeamBroadcast' }); | ||
| throw new Error(`Failed to update broadcast recipients: ${syncError.message}`); | ||
| } | ||
| } | ||
|
|
||
| return { conversationId: reuseTarget.id, reused: true }; | ||
| } |
There was a problem hiding this comment.
6. Broadcast audience leaks on reuse 🐞 Bug ⛨ Security
createGolfTeamBroadcastImpl reuses an existing same-titled team chat even when the newly selectedPlayerIds imply a different audience, and it only adds missing participants without removing stale ones—so future broadcasts can be delivered to unintended players from the prior audience.
Agent Prompt
### Issue description
When a broadcast title already exists, the code reuses the oldest conversation even if the participant set differs, and only inserts `missingIds` while explicitly never removing existing participants. This can cause messages meant for a newly selected subset (e.g., JV) to also be delivered to prior recipients (e.g., Varsity) who remain participants.
### Issue Context
- `selectedPlayerIds` is explicitly supported to target a specific subset.
- Conversation participants define who can see/receive messages in that thread.
- The current logic makes same-title threads accumulate participants over time (union of audiences).
### Fix Focus Areas
- src/app/actions/messages.ts[503-651]
### Implementation notes
Choose one:
1) **Strict audience semantics (recommended):** if no exact participant-set match exists, create a NEW conversation (no reuse).
2) If reuse is required, reconcile to the new audience by removing stale participants (requires confirming product + ensuring non-destructive constraints don’t conflict), and ensure the coach is always included.
Also consider surfacing `reused` semantics to the UI so coaches understand whether they’re posting into an old thread.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+379
to
+449
| const { error: updateError } = await supabase | ||
| .from('baseball_team_members') | ||
| .update({ | ||
| status: 'active', | ||
| approved_by: coach?.id ?? null, | ||
| approved_at: new Date().toISOString(), | ||
| }) | ||
| .eq('id', memberId); | ||
|
|
||
| if (updateError) { | ||
| throw updateError; | ||
| } | ||
|
|
||
| const playerRow = (membership as unknown as { | ||
| baseball_players: { first_name: string | null; last_name: string | null } | null; | ||
| }).baseball_players; | ||
|
|
||
| // Best-effort timeline side-effect — never rolls back the approval. | ||
| await appendRosterTimelineEvent({ | ||
| teamId: membership.team_id, | ||
| playerId: membership.player_id, | ||
| title: `${playerDisplayName(playerRow)}'s join request was approved`, | ||
| visibility: 'team', | ||
| source: 'manual', | ||
| createdBy: ctx.user.id, | ||
| }); | ||
|
|
||
| revalidateRoster(); | ||
| return { success: true, memberId }; | ||
| }, | ||
| ); | ||
|
|
||
| /** | ||
| * Decline a pending join request. This is a scoped single-membership DELETE | ||
| * (never the player's account/stats) — the player can rejoin later via the | ||
| * team's invite link if the decline was in error. | ||
| */ | ||
| export const rejectPendingMember = withBaseballAction( | ||
| 'rejectPendingMember', | ||
| { featureArea: 'baseball-roster' }, | ||
| async (ctx, args: { memberId: string }): Promise<PendingMemberActionResult> => { | ||
| const { memberId } = args; | ||
| if (!memberId) { | ||
| return { success: false, error: 'A membership is required.' }; | ||
| } | ||
|
|
||
| const supabase = await createClient(); | ||
| const { data: membership, error: memberError } = await loadPendingMembership(supabase, memberId); | ||
|
|
||
| if (memberError || !membership) { | ||
| return { success: false, error: 'That join request could not be found.' }; | ||
| } | ||
|
|
||
| try { | ||
| await requireBaseballCapability(membership.team_id, 'can_manage_roster'); | ||
| } catch { | ||
| return { success: false, error: 'You do not have permission to manage this roster.' }; | ||
| } | ||
|
|
||
| if (membership.status !== 'pending') { | ||
| return { success: false, error: 'This request has already been processed.' }; | ||
| } | ||
|
|
||
| const playerRow = (membership as unknown as { | ||
| baseball_players: { first_name: string | null; last_name: string | null } | null; | ||
| }).baseball_players; | ||
|
|
||
| const { error: deleteError } = await supabase | ||
| .from('baseball_team_members') | ||
| .delete() | ||
| .eq('id', memberId); |
There was a problem hiding this comment.
7. Pending join writes not conditional 🐞 Bug ☼ Reliability
approvePendingMember and rejectPendingMember check membership.status in a prior read but then
UPDATE/DELETE by id only; without a conditional predicate (e.g., eq('status','pending')) and
affected-row verification, concurrent processing can act on stale state and incorrectly
update/delete an already-processed membership.
Agent Prompt
### Issue description
`approvePendingMember` and `rejectPendingMember` read the row (including `status`) and then write by `id` only. Under concurrency/double-submit, both requests can pass the `status === 'pending'` check and then both mutate the row because the mutation isn’t constrained to `status='pending'`.
### Issue Context
These are admin/coach actions that are realistic to double-click or run concurrently by multiple staff.
### Fix Focus Areas
- src/app/baseball/actions/roster.ts[369-390]
- src/app/baseball/actions/roster.ts[438-453]
### Implementation notes
- For approval: `...update({...}).eq('id', memberId).eq('status','pending').select('id').maybeSingle()` and error if no row updated.
- For rejection: `...delete().eq('id', memberId).eq('status','pending').select('id')` and error if no row deleted.
- Optionally add an idempotent response if the row is already processed, but ensure you don’t delete/overwrite non-pending rows.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/baseball/read-models/roster.ts (1)
130-164: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize the two independent aggregate queries.
baseball_player_aggregatesandbaseball_player_season_statsonly depend onteamIdand are fetched sequentially. Run them viaPromise.allto cut one round trip off this read path, mirroring thePromise.allpattern already used elsewhere in this PR (e.g.lift/page.tsx'sfetchPlayerSessions).⚡ Suggested parallelization
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const { data: aggregatesData, error: aggregatesError } = await (supabase as any) - .from('baseball_player_aggregates') - .select('*') - .eq('team_id', teamId) as { - data: BaseballPlayerAggregates[] | null; - error: unknown; - }; - - let aggregates: Record<string, BaseballPlayerAggregates> = {}; - if (!aggregatesError && aggregatesData) { - for (const agg of aggregatesData) { - aggregates[agg.player_id] = agg; - } - } - - const currentSeasonYear = new Date().getFullYear(); - const { data: seasonStatsData, error: seasonStatsError } = await supabase - .from('baseball_player_season_stats') - .select('player_id, avg, obp, slg, ops, g, last_updated') - .eq('team_id', teamId) - .eq('season_year', currentSeasonYear); + const currentSeasonYear = new Date().getFullYear(); + const [ + // eslint-disable-next-line `@typescript-eslint/no-explicit-any` + { data: aggregatesData, error: aggregatesError }, + { data: seasonStatsData, error: seasonStatsError }, + ] = await Promise.all([ + (supabase as any) + .from('baseball_player_aggregates') + .select('*') + .eq('team_id', teamId) as Promise<{ + data: BaseballPlayerAggregates[] | null; + error: unknown; + }>, + supabase + .from('baseball_player_season_stats') + .select('player_id, avg, obp, slg, ops, g, last_updated') + .eq('team_id', teamId) + .eq('season_year', currentSeasonYear), + ]); + + let aggregates: Record<string, BaseballPlayerAggregates> = {}; + if (!aggregatesError && aggregatesData) { + for (const agg of aggregatesData) { + aggregates[agg.player_id] = agg; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/baseball/read-models/roster.ts` around lines 130 - 164, Parallelize the independent queries in the roster read path by starting the baseball_player_aggregates and baseball_player_season_stats requests together with Promise.all, then destructure their results and preserve the existing error handling and merge behavior in mergeSeasonStatsIntoAggregates.src/components/fairway/pages/calendar/FairwayMonthGrid.tsx (1)
216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCancelled cue is tooltip-only — not exposed to assistive tech.
titlerenders"${e.title} (cancelled)"but that string is never surfaced as an accessible name; screen readers announce only the visible child text (e.title), so the cancelled status is invisible non-visually.♿️ Proposed fix
onClick={onEventClick ? () => onEventClick(e) : undefined} title={isCancelled ? `${e.title} (cancelled)` : e.title} + aria-label={isCancelled ? `${e.title} (cancelled)` : undefined} className={cn(As per path instructions, "Components handling user input must have visible focus rings, aria-labels, and keyboard handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/fairway/pages/calendar/FairwayMonthGrid.tsx` around lines 216 - 236, Expose the cancelled status to assistive technology for each event Button rendered in the calendar grid. Update the Button’s accessible label, using the same title-with-cancelled-suffix logic currently used by title, while preserving the visible event text and existing click/keyboard behavior; ensure the Button retains a visible focus ring consistent with the component’s accessibility requirements.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/actions/messages.ts`:
- Around line 401-414: Fix the golf message-read failure logging in the messages
action: since messagesError is typed as an object with a message field, the
downstream instanceof Error check produces “[object Object]”. Update the
non-fatal log to use messagesError.message directly, while preserving null
handling and the existing baseball path.
In `@src/app/golf/actions/message-attachments.ts`:
- Around line 135-144: Isolate notifyGolfMessageRecipients from the outer send
try/catch so notification failures cannot turn an already-persisted message into
a failed result. Catch and log fan-out errors after the message insert succeeds,
while still returning the successful send response and preserving the existing
message ID.
In `@src/app/golf/actions/tasks.ts`:
- Around line 465-488: The reminder replacement in setTaskReminder is not atomic
because delete, insert, and task update occur in separate operations. Refactor
setTaskReminder to perform the pending golf_task_reminders deletion, replacement
insertion, and related golf_tasks update through one transactional RPC or
database transaction, preserving error handling and returning failure if any
step rolls back.
In
`@src/components/baseball/practice-planner/__tests__/PracticeRecapPanel.test.tsx`:
- Around line 94-96: Update the Cancel button assertion in PracticeRecapPanel
tests to require exactly two matching buttons, replacing the current
greater-than-or-equal-to-one check so it verifies both the composer and panel
row remain inline.
In `@src/components/fairway/pages/qualifiers/FairwayQualifyingWorkspace.tsx`:
- Line 160: Update ConcludeQualifier’s status prop to use the existing
QualifierStatus-style union type from the qualifying types module instead of
string, and ensure the workspace.status value and its comparison with
'completed' use that narrowed type.
In `@src/lib/baseball/read-models/__tests__/roster-aggregates-merge.test.ts`:
- Around line 61-76: Add a test alongside the existing
mergeSeasonStatsIntoAggregates case that uses a legacy aggregate with
total_sessions lower than the season-stats g value, then assert the merged
player’s total_sessions equals the box-score g value. Keep the setup focused on
the same player and preserve assertions that validate the MAX behavior.
In `@src/lib/coachhelm/v3/qualifying/player-notify.ts`:
- Around line 40-54: Collapse the sequential player and user queries in the
notification logic into one Supabase query by selecting the player fields and
embedding the related user’s id and email through the relationship. Update the
result transformation to build the required player-to-user and user-to-email
mappings from the nested response, preserving the existing early return behavior
when no rows or related users are present.
In
`@supabase/migrations/20260710020000_baseball_settings_audit_log_column_reconcile.sql`:
- Around line 1-10: Reword the header comment in the migration so it no longer
contains the exact CREATE TABLE phrase targeted by semgrep, while preserving the
explanation that an existing older-schema table prevented the intended
definition from being applied. Do not change the ALTER TABLE or
policy-reconciliation SQL.
- Around line 37-43: Add a dedicated index for the foreign-key column
actor_coach_id after the ALTER TABLE statement, using a clear name consistent
with the existing indexes. Keep the foreign key definition unchanged and ensure
the index is created idempotently with CREATE INDEX IF NOT EXISTS.
In `@supabase/migrations/20260710031500_baseball_decision_log_kind_reconcile.sql`:
- Around line 6-9: Reword the root-cause comment in the migration to remove the
literal “CREATE TABLE IF NOT EXISTS” DDL pattern, while preserving the
explanation that the earlier migration’s conditional table creation was skipped
because an unrelated legacy table already existed. Do not change the migration’s
ALTER TABLE, constraint, or index statements.
---
Outside diff comments:
In `@src/components/fairway/pages/calendar/FairwayMonthGrid.tsx`:
- Around line 216-236: Expose the cancelled status to assistive technology for
each event Button rendered in the calendar grid. Update the Button’s accessible
label, using the same title-with-cancelled-suffix logic currently used by title,
while preserving the visible event text and existing click/keyboard behavior;
ensure the Button retains a visible focus ring consistent with the component’s
accessibility requirements.
In `@src/lib/baseball/read-models/roster.ts`:
- Around line 130-164: Parallelize the independent queries in the roster read
path by starting the baseball_player_aggregates and baseball_player_season_stats
requests together with Promise.all, then destructure their results and preserve
the existing error handling and merge behavior in
mergeSeasonStatsIntoAggregates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c41a0080-8edb-4a6f-a230-e7d410c80497
⛔ Files ignored due to path filters (1)
src/lib/types/database.tsis excluded by!src/lib/types/database.ts
📒 Files selected for processing (94)
src/app/actions/messages.tssrc/app/baseball/(dashboard)/dashboard/academics/AcademicsClient.tsxsrc/app/baseball/(dashboard)/dashboard/academics/ClassScheduleModal.tsxsrc/app/baseball/(dashboard)/dashboard/compare/CompareClient.tsxsrc/app/baseball/(dashboard)/dashboard/compare/__tests__/search-recruitability.test.tssrc/app/baseball/(dashboard)/dashboard/compare/actions.tssrc/app/baseball/(dashboard)/dashboard/lift/page.tsxsrc/app/baseball/(dashboard)/dashboard/roster/RosterClient.tsxsrc/app/baseball/(dashboard)/dashboard/roster/RosterFairway.tsxsrc/app/baseball/(dashboard)/dashboard/roster/RosterMemberActions.tsxsrc/app/baseball/(dashboard)/dashboard/roster/SavedLineupsPanel.tsxsrc/app/baseball/(dashboard)/dashboard/watchlist/WatchlistClient.tsxsrc/app/baseball/actions/__tests__/academics-coach-gating.test.tssrc/app/baseball/actions/__tests__/assign-player-membership-fields.test.tssrc/app/baseball/actions/__tests__/decision-log-column-reconcile.test.tssrc/app/baseball/actions/__tests__/dev-plans-coach-gating.test.tssrc/app/baseball/actions/__tests__/imports-event-grain-guard.test.tssrc/app/baseball/actions/__tests__/imports-registry.test.tssrc/app/baseball/actions/academics.tssrc/app/baseball/actions/decision-room.tssrc/app/baseball/actions/dev-plans.tssrc/app/baseball/actions/imports.tssrc/app/baseball/actions/practice-planner-capability.tssrc/app/baseball/actions/practice.tssrc/app/baseball/actions/program-settings.tssrc/app/baseball/actions/roster.tssrc/app/golf/(dashboard)/dashboard/qualifiers/[id]/edit/error.tsxsrc/app/golf/(dashboard)/dashboard/qualifiers/[id]/edit/loading.tsxsrc/app/golf/(dashboard)/dashboard/qualifiers/[id]/edit/page.tsxsrc/app/golf/(dashboard)/dashboard/qualifiers/[id]/page.tsxsrc/app/golf/(dashboard)/dashboard/recruiting/page.tsxsrc/app/golf/(dashboard)/dashboard/rounds/[id]/review/CoachNotesSection.tsxsrc/app/golf/(dashboard)/dashboard/rounds/[id]/review/__tests__/CoachNotesSection.test.tsxsrc/app/golf/(dashboard)/dashboard/rounds/[id]/review/page.tsxsrc/app/golf/actions/__tests__/golf-save-partial-round.test.tssrc/app/golf/actions/__tests__/recurring-events.test.tssrc/app/golf/actions/development.tssrc/app/golf/actions/golf.tssrc/app/golf/actions/message-attachments.tssrc/app/golf/actions/recruiting.tssrc/app/golf/actions/recurring-events.tssrc/app/golf/actions/round-reviews.tssrc/app/golf/actions/tasks.tssrc/app/golf/actions/travel.tssrc/app/lifting/(dashboard)/dashboard/lift/page.tsxsrc/components/baseball/import-center/ImportWizardClient.tsxsrc/components/baseball/player-profile/PlayerNotesSection.tsxsrc/components/baseball/player-profile/PlayerProfileClient.tsxsrc/components/baseball/practice-planner/PracticePlannerClient.tsxsrc/components/baseball/practice-planner/PracticePrintExport.tsxsrc/components/baseball/practice-planner/PracticeRecapPanel.tsxsrc/components/baseball/practice-planner/__tests__/PracticeRecapPanel.test.tsxsrc/components/coach/lineup/LineupBuilder.tsxsrc/components/fairway/pages/calendar/FairwayEventCard.tsxsrc/components/fairway/pages/calendar/FairwayMonthGrid.tsxsrc/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsxsrc/components/fairway/pages/coachhelm/FocusAreaCard.tsxsrc/components/fairway/pages/coachhelm/PlayersGridView.tsxsrc/components/fairway/pages/qualifiers/FairwayEditQualifier.tsxsrc/components/fairway/pages/qualifiers/FairwayQualifierDetail.tsxsrc/components/fairway/pages/qualifiers/FairwayQualifierLeaderboard.tsxsrc/components/fairway/pages/qualifiers/FairwayQualifyingWorkspace.tsxsrc/components/fairway/pages/roster/FairwayJoinRequests.test.tsxsrc/components/fairway/pages/roster/FairwayJoinRequests.tsxsrc/components/fairway/pages/settings/FairwaySettingsCoachingIntelligence.tsxsrc/components/golf/calendar/PremiumCalendarClient.tsxsrc/components/lifting/players/PlayerLiftHomeClient.tsxsrc/components/lifting/players/__tests__/PlayerLiftHomeClient.test.tsxsrc/lib/admin/__tests__/coverage-contract.foundation.test.tssrc/lib/admin/__tests__/feature-registry.test.tssrc/lib/admin/__tests__/github-pr-timeline.test.tssrc/lib/admin/feature-registry.tssrc/lib/admin/github-pr-timeline.tssrc/lib/baseball/nav-registry.tssrc/lib/baseball/read-models/__tests__/roster-aggregates-merge.test.tssrc/lib/baseball/read-models/roster-aggregates-merge.tssrc/lib/baseball/read-models/roster.tssrc/lib/baseball/stat-layer-manifest.tssrc/lib/coachhelm/v3/qualifying/loader.tssrc/lib/coachhelm/v3/qualifying/player-notify.tssrc/lib/coachhelm/v3/qualifying/service.tssrc/lib/coachhelm/v3/qualifying/types.tssrc/lib/notifications/golf-message-fanout.tssrc/lib/notifications/push.tssrc/test/coachhelm/v3/confirm-selection-chat.integration.test.tssrc/test/coachhelm/v3/player-notify.test.tssrc/test/coachhelm/v3/travel-brief.test.tssrc/test/fairway/settings-coaching-intelligence.test.tsxsrc/test/golf/actions/development.test.tssrc/test/lib/notifications/push.test.tssupabase/migrations/20260710020000_baseball_settings_audit_log_column_reconcile.sqlsupabase/migrations/20260710031500_baseball_decision_log_kind_reconcile.sqlsupabase/migrations/20260710150000_baseball_practice_blocks_visibility_rls.sqlsupabase/migrations/20260710160000_mark_golf_messages_read_rpc.sql
Comment on lines
+401
to
+414
| let messagesError: { message: string } | null = null; | ||
| if (sport === 'golf') { | ||
| const { error } = await (supabase as any).rpc('mark_golf_messages_read', { | ||
| p_conversation_id: conversationId, | ||
| }); | ||
| messagesError = error; | ||
| } else { | ||
| const { error } = await supabase | ||
| .from('baseball_messages' as any) | ||
| .update({ read: true }) | ||
| .eq('conversation_id', conversationId) | ||
| .neq('sender_id', user.id); | ||
| messagesError = error; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Error message is lost in the log on the golf-RPC failure path.
messagesError is typed { message: string } | null, so the downstream log at Line 417 (messagesError instanceof Error ? messagesError.message : String(messagesError)) always hits the String() branch and emits [object Object]. That defeats the "logged as non-fatal" observability this golf change explicitly relies on. Log messagesError.message directly.
🔭 Fix (line 417)
- await logServerError(`[Messages] Failed to mark messages as read: ${messagesError instanceof Error ? messagesError.message : String(messagesError)}`, { action: 'messages.markMessagesAsRead' });
+ await logServerError(`[Messages] Failed to mark messages as read: ${messagesError.message}`, { action: 'messages.markMessagesAsRead' });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/actions/messages.ts` around lines 401 - 414, Fix the golf
message-read failure logging in the messages action: since messagesError is
typed as an object with a message field, the downstream instanceof Error check
produces “[object Object]”. Update the non-fatal log to use
messagesError.message directly, while preserving null handling and the existing
baseball path.
Comment on lines
+135
to
+144
| // Fan out email/push/in-app notifications to the other participants — | ||
| // mirrors sendGolfMessageImpl's text-only fan-out (P1: this attachments | ||
| // path previously fired none of these, so a photo/document send was | ||
| // invisible to the recipient until they manually opened Messages). | ||
| await notifyGolfMessageRecipients( | ||
| conversationId, | ||
| user.id, | ||
| buildAttachmentPreview(content, attachments), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Notification failure will falsely report the whole send as failed.
notifyGolfMessageRecipients (Line 139-143) runs inside the same try that already committed the message row (Line 87-97). If it throws, control falls to the outer catch (Line 146), which returns { success: false, error: ... } — even though the message was already persisted. A client that retries on success: false will re-insert a duplicate message.
🔧 Proposed fix: isolate fan-out failure from the send result
- await notifyGolfMessageRecipients(
- conversationId,
- user.id,
- buildAttachmentPreview(content, attachments),
- );
+ try {
+ await notifyGolfMessageRecipients(
+ conversationId,
+ user.id,
+ buildAttachmentPreview(content, attachments),
+ );
+ } catch (notifyErr) {
+ await logServerError(
+ `[Attachments] Notification fan-out failed: ${notifyErr instanceof Error ? notifyErr.message : String(notifyErr)}`,
+ { action: 'message_attachments.sendGolfMessageWithAttachments' },
+ );
+ // Message already persisted — don't fail the whole send over a notify error.
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fan out email/push/in-app notifications to the other participants — | |
| // mirrors sendGolfMessageImpl's text-only fan-out (P1: this attachments | |
| // path previously fired none of these, so a photo/document send was | |
| // invisible to the recipient until they manually opened Messages). | |
| await notifyGolfMessageRecipients( | |
| conversationId, | |
| user.id, | |
| buildAttachmentPreview(content, attachments), | |
| ); | |
| // Fan out email/push/in-app notifications to the other participants — | |
| // mirrors sendGolfMessageImpl's text-only fan-out (P1: this attachments | |
| // path previously fired none of these, so a photo/document send was | |
| // invisible to the recipient until they manually opened Messages). | |
| try { | |
| await notifyGolfMessageRecipients( | |
| conversationId, | |
| user.id, | |
| buildAttachmentPreview(content, attachments), | |
| ); | |
| } catch (notifyErr) { | |
| await logServerError( | |
| `[Attachments] Notification fan-out failed: ${notifyErr instanceof Error ? notifyErr.message : String(notifyErr)}`, | |
| { action: 'message_attachments.sendGolfMessageWithAttachments' }, | |
| ); | |
| // Message already persisted — don't fail the whole send over a notify error. | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/golf/actions/message-attachments.ts` around lines 135 - 144, Isolate
notifyGolfMessageRecipients from the outer send try/catch so notification
failures cannot turn an already-persisted message into a failed result. Catch
and log fan-out errors after the message insert succeeds, while still returning
the successful send response and preserving the existing message ID.
Comment on lines
+465
to
+488
| const { error: clearQueueError } = await supabase | ||
| .from('golf_task_reminders') | ||
| .delete() | ||
| .eq('task_id', taskId) | ||
| .eq('sent', false); | ||
|
|
||
| if (clearQueueError) { | ||
| await logServerError(`[setTaskReminder Error]: ${clearQueueError instanceof Error ? clearQueueError.message : String(clearQueueError)}`, { action: 'tasks.setTaskReminder' }); | ||
| return { success: false, error: clearQueueError.message }; | ||
| } | ||
|
|
||
| const { error: queueError } = await supabase | ||
| .from('golf_task_reminders') | ||
| .insert({ | ||
| task_id: taskId, | ||
| scheduled_for: reminderAt, | ||
| reminder_type: 'in_app', | ||
| sent: false, | ||
| }); | ||
|
|
||
| if (queueError) { | ||
| await logServerError(`[setTaskReminder Error]: ${queueError instanceof Error ? queueError.message : String(queueError)}`, { action: 'tasks.setTaskReminder' }); | ||
| return { success: false, error: queueError.message }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the review gate flag delete->insert on the same table here?
fd -e yml -e yaml . .coderabbit .greptile 2>/dev/null | xargs rg -n -i 'delete.*insert|destructive' 2>/dev/null
rg -n "mark_golf_messages_read|task_reminders" -g '*.sql' supabase/migrationsRepository: njrini99-code/helmv3
Length of output: 1992
Wrap the reminder replacement in one transaction src/app/golf/actions/tasks.ts:465-499 deletes the pending golf_task_reminders row and inserts a replacement in separate round-trips. If the insert fails after the delete succeeds, the reminder is lost, and the later golf_tasks update can still leave the UI showing a reminder that will never fire. Move the delete/insert/update into one atomic RPC or transaction.
🧰 Tools
🪛 GitHub Actions: Review Gate / 10_semgrep (custom rules).txt
[error] 465-474: coderabbit.semgrep.helmv3-destructive-write-pattern: DELETE followed by INSERT/clear path on the same table in a save/sync/submit path (risk of permanent data loss).
🪛 GitHub Actions: Review Gate / semgrep (custom rules)
[error] 465-474: Semgrep (coderabbit.semgrep.helmv3-destructive-write-pattern): Blocking. DELETE followed by INSERT on the same table in a save/sync/submit path (risk of permanent data loss on transient failure).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/golf/actions/tasks.ts` around lines 465 - 488, The reminder
replacement in setTaskReminder is not atomic because delete, insert, and task
update occur in separate operations. Refactor setTaskReminder to perform the
pending golf_task_reminders deletion, replacement insertion, and related
golf_tasks update through one transactional RPC or database transaction,
preserving error handling and returning failure if any step rolls back.
Comment on lines
+94
to
+96
| expect(screen.getByText('First-step reads')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: /save recap/i })).toBeInTheDocument(); | ||
| expect(screen.getAllByRole('button', { name: 'Cancel' }).length).toBeGreaterThanOrEqual(1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assertion doesn't verify the documented invariant.
The comment states two "Cancel" buttons must coexist (composer's own + panel's row) as proof the composer stayed inline, but toBeGreaterThanOrEqual(1) passes even if the composer collapsed to a single Cancel. Assert the exact count to make the regression guard match its intent.
💚 Tighten the guard
- expect(screen.getAllByRole('button', { name: 'Cancel' }).length).toBeGreaterThanOrEqual(1);
+ expect(screen.getAllByRole('button', { name: 'Cancel' })).toHaveLength(2);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(screen.getByText('First-step reads')).toBeInTheDocument(); | |
| expect(screen.getByRole('button', { name: /save recap/i })).toBeInTheDocument(); | |
| expect(screen.getAllByRole('button', { name: 'Cancel' }).length).toBeGreaterThanOrEqual(1); | |
| expect(screen.getByText('First-step reads')).toBeInTheDocument(); | |
| expect(screen.getByRole('button', { name: /save recap/i })).toBeInTheDocument(); | |
| expect(screen.getAllByRole('button', { name: 'Cancel' })).toHaveLength(2); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/baseball/practice-planner/__tests__/PracticeRecapPanel.test.tsx`
around lines 94 - 96, Update the Cancel button assertion in PracticeRecapPanel
tests to require exactly two matching buttons, replacing the current
greater-than-or-equal-to-one check so it verifies both the composer and panel
row remain inline.
| * golf.ts) — this button is the manual override for a coach who doesn't want | ||
| * to wait for stragglers, or who never lets that auto-transition fire. | ||
| * ────────────────────────────────────────────────────────────────────────── */ | ||
| function ConcludeQualifier({ qualifierId, status }: { qualifierId: string; status: string }) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Widen-then-narrow: status: string throws away the real union type.
ConcludeQualifier's prop is typed as bare string at Line 160, but workspace.status (destructured at Line 84) almost certainly comes from a typed QualifierStatus-style union in @/lib/coachhelm/v3/qualifying/types. Comparing a widened string against the literal 'completed' at Line 164 means a typo like 'compelted' would compile silently instead of erroring.
♻️ Proposed fix
+import type { QualifierStatus } from '`@/lib/coachhelm/v3/qualifying/types`';
-function ConcludeQualifier({ qualifierId, status }: { qualifierId: string; status: string }) {
+function ConcludeQualifier({ qualifierId, status }: { qualifierId: string; status: QualifierStatus }) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function ConcludeQualifier({ qualifierId, status }: { qualifierId: string; status: string }) { | |
| import type { QualifierStatus } from '`@/lib/coachhelm/v3/qualifying/types`'; | |
| function ConcludeQualifier({ qualifierId, status }: { qualifierId: string; status: QualifierStatus }) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/fairway/pages/qualifiers/FairwayQualifyingWorkspace.tsx` at
line 160, Update ConcludeQualifier’s status prop to use the existing
QualifierStatus-style union type from the qualifying types module instead of
string, and ensure the workspace.status value and its comparison with
'completed' use that narrowed type.
Comment on lines
+61
to
+76
| it('prefers season-stats figures over a legacy aggregates row for the same player', () => { | ||
| const legacy = { p1: legacyAggregate({ career_avg: 0.1, career_ops: 0.3, total_sessions: 40 }) }; | ||
|
|
||
| const merged = mergeSeasonStatsIntoAggregates( | ||
| legacy, | ||
| [{ player_id: 'p1', avg: 0.35, obp: 0.42, slg: 0.55, ops: 0.97, g: 20, last_updated: '2026-06-15T00:00:00.000Z' }], | ||
| 'team1', | ||
| ); | ||
|
|
||
| // Season-stats rate figures win — they're the box-score-canonical source. | ||
| expect(merged.p1?.career_avg).toBe(0.35); | ||
| expect(merged.p1?.career_ops).toBe(0.97); | ||
| // total_sessions takes the MAX of legacy vs. box-score game count, so a | ||
| // team with a longer legacy history never regresses to a smaller number. | ||
| expect(merged.p1?.total_sessions).toBe(40); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add the missing MAX-direction test case.
Only the "legacy total_sessions > box-score g" direction is asserted (40 vs 20 → 40). Add a case where box-score g exceeds legacy total_sessions to lock the other half of the MAX contract described in the surrounding comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/baseball/read-models/__tests__/roster-aggregates-merge.test.ts`
around lines 61 - 76, Add a test alongside the existing
mergeSeasonStatsIntoAggregates case that uses a legacy aggregate with
total_sessions lower than the season-stats g value, then assert the merged
player’s total_sessions equals the box-score g value. Keep the setup focused on
the same player and preserve assertions that validate the MAX behavior.
Comment on lines
+40
to
+54
| const { data: playerRows } = await supabase | ||
| .from('golf_players') | ||
| .select('id, user_id') | ||
| .in('id', playerIds); | ||
| if (!playerRows?.length) return; | ||
|
|
||
| const userIdByPlayer = new Map(playerRows.map((p) => [p.id, p.user_id])); | ||
|
|
||
| const { data: userRows } = await supabase | ||
| .from('users') | ||
| .select('id, email') | ||
| .in('id', playerRows.map((p) => p.user_id)); | ||
| if (!userRows?.length) return; | ||
|
|
||
| const emailByUser = new Map(userRows.map((u) => [u.id, u.email])); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Two round-trips that could be one join.
golf_players (Lines 40-43) then users (Lines 48-51) are fetched sequentially even though the second query's .in() list is entirely derived from the first result. Supabase supports embedded resource selection — collapsing this into a single query with a nested select removes one network round-trip on a path that's already inside a best-effort Promise.allSettled fan-out.
♻️ Proposed fix
- const { data: playerRows } = await supabase
- .from('golf_players')
- .select('id, user_id')
- .in('id', playerIds);
- if (!playerRows?.length) return;
-
- const userIdByPlayer = new Map(playerRows.map((p) => [p.id, p.user_id]));
-
- const { data: userRows } = await supabase
- .from('users')
- .select('id, email')
- .in('id', playerRows.map((p) => p.user_id));
- if (!userRows?.length) return;
-
- const emailByUser = new Map(userRows.map((u) => [u.id, u.email]));
+ const { data: playerRows } = await supabase
+ .from('golf_players')
+ .select('id, user_id, users(id, email)')
+ .in('id', playerIds);
+ if (!playerRows?.length) return;
+
+ const userIdByPlayer = new Map(playerRows.map((p) => [p.id, p.user_id]));
+ const emailByUser = new Map(
+ playerRows
+ .filter((p) => p.users)
+ .map((p) => [p.users!.id, p.users!.email]),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { data: playerRows } = await supabase | |
| .from('golf_players') | |
| .select('id, user_id') | |
| .in('id', playerIds); | |
| if (!playerRows?.length) return; | |
| const userIdByPlayer = new Map(playerRows.map((p) => [p.id, p.user_id])); | |
| const { data: userRows } = await supabase | |
| .from('users') | |
| .select('id, email') | |
| .in('id', playerRows.map((p) => p.user_id)); | |
| if (!userRows?.length) return; | |
| const emailByUser = new Map(userRows.map((u) => [u.id, u.email])); | |
| const { data: playerRows } = await supabase | |
| .from('golf_players') | |
| .select('id, user_id, users(id, email)') | |
| .in('id', playerIds); | |
| if (!playerRows?.length) return; | |
| const userIdByPlayer = new Map(playerRows.map((p) => [p.id, p.user_id])); | |
| const emailByUser = new Map( | |
| playerRows | |
| .filter((p) => p.users) | |
| .map((p) => [p.users!.id, p.users!.email]), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/coachhelm/v3/qualifying/player-notify.ts` around lines 40 - 54,
Collapse the sequential player and user queries in the notification logic into
one Supabase query by selecting the player fields and embedding the related
user’s id and email through the relationship. Update the result transformation
to build the required player-to-user and user-to-email mappings from the nested
response, preserving the existing early return behavior when no rows or related
users are present.
Comment on lines
+37
to
+43
| ALTER TABLE public.baseball_settings_audit_log | ||
| ADD COLUMN IF NOT EXISTS actor_user_id uuid, | ||
| ADD COLUMN IF NOT EXISTS actor_coach_id uuid REFERENCES public.baseball_coaches(id) ON DELETE SET NULL, | ||
| ADD COLUMN IF NOT EXISTS event_type text, | ||
| ADD COLUMN IF NOT EXISTS summary text, | ||
| ADD COLUMN IF NOT EXISTS before_value jsonb, | ||
| ADD COLUMN IF NOT EXISTS after_value jsonb; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Missing index on new FK column actor_coach_id.
actor_coach_id is added as a foreign key to baseball_coaches(id) but no covering index is created (only (team_id, event_type) gets one). Per migration path instructions, FK columns should be indexed to avoid slow joins and ON DELETE SET NULL scans on baseball_coaches deletes.
🔧 Proposed fix
CREATE INDEX IF NOT EXISTS baseball_settings_audit_log_event_type_idx
ON public.baseball_settings_audit_log (team_id, event_type);
+
+CREATE INDEX IF NOT EXISTS baseball_settings_audit_log_actor_coach_id_idx
+ ON public.baseball_settings_audit_log (actor_coach_id);As per path instructions, "Add indexes on FK columns and on every column used in RLS predicates."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ALTER TABLE public.baseball_settings_audit_log | |
| ADD COLUMN IF NOT EXISTS actor_user_id uuid, | |
| ADD COLUMN IF NOT EXISTS actor_coach_id uuid REFERENCES public.baseball_coaches(id) ON DELETE SET NULL, | |
| ADD COLUMN IF NOT EXISTS event_type text, | |
| ADD COLUMN IF NOT EXISTS summary text, | |
| ADD COLUMN IF NOT EXISTS before_value jsonb, | |
| ADD COLUMN IF NOT EXISTS after_value jsonb; | |
| ALTER TABLE public.baseball_settings_audit_log | |
| ADD COLUMN IF NOT EXISTS actor_user_id uuid, | |
| ADD COLUMN IF NOT EXISTS actor_coach_id uuid REFERENCES public.baseball_coaches(id) ON DELETE SET NULL, | |
| ADD COLUMN IF NOT EXISTS event_type text, | |
| ADD COLUMN IF NOT EXISTS summary text, | |
| ADD COLUMN IF NOT EXISTS before_value jsonb, | |
| ADD COLUMN IF NOT EXISTS after_value jsonb; | |
| CREATE INDEX IF NOT EXISTS baseball_settings_audit_log_actor_coach_id_idx | |
| ON public.baseball_settings_audit_log (actor_coach_id); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@supabase/migrations/20260710020000_baseball_settings_audit_log_column_reconcile.sql`
around lines 37 - 43, Add a dedicated index for the foreign-key column
actor_coach_id after the ALTER TABLE statement, using a clear name consistent
with the existing indexes. Keep the foreign key definition unchanged and ensure
the index is created idempotently with CREATE INDEX IF NOT EXISTS.
Source: Path instructions
…ve task-reminder re-arm, annotate the guarded scorecard swap - push.ts: same functions.invoke-through-admin-client change as #798 (3-way merged under this branch's preference-routing rewrite) so the raw service-role header is gone on both branches and they merge clean. - messages.ts: same nosemgrep annotation as #798 (realtime-subscribed UI, intentional no-revalidate). - tasks.ts setTaskReminder: update-or-insert re-arm of the unsent queue row instead of delete-then-insert — a transient failure can no longer leave a task with no armed reminder. - golf.ts fallback swap: nosemgrep annotations — the flagged deletes are the snapshot-guarded swap (null-checked snapshot captured before any delete; every failure path restores) and its rollback helper itself. semgrep exit 0 (unpiped), tsc, eslint, 1,408 targeted tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/golf/actions/tasks.ts (1)
458-491: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd a uniqueness guard for armed task reminders
src/app/golf/actions/tasks.ts:467-491still does a read-then-insert/update ongolf_task_reminders, and the schema only has non-unique indexes (supabase/migrations/20260527000000_prod_public_baseline.sql:14245-14253) — no partial unique constraint on(task_id) WHERE sent = false. Two concurrentsetTaskRemindercalls can create duplicate unsent rows, andsrc/app/golf/actions/task-reminders.ts:251-258will pick both up for dispatch. Add a partial unique index, or switch to an atomic upsert path keyed to the unsent reminder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/golf/actions/tasks.ts` around lines 458 - 491, Add a database-level uniqueness guard for unsent reminders, preferably a partial unique index on golf_task_reminders(task_id) WHERE sent = false, and update setTaskReminder to use an atomic upsert targeting that constraint instead of the read-then-insert/update flow around existingQueueRow. Ensure concurrent calls leave exactly one armed reminder and preserve the existing error handling via queueError.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/app/golf/actions/tasks.ts`:
- Around line 458-491: Add a database-level uniqueness guard for unsent
reminders, preferably a partial unique index on golf_task_reminders(task_id)
WHERE sent = false, and update setTaskReminder to use an atomic upsert targeting
that constraint instead of the read-then-insert/update flow around
existingQueueRow. Ensure concurrent calls leave exactly one armed reminder and
preserve the existing error handling via queueError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 40644373-24fd-4dd8-813d-f659d13a3145
📒 Files selected for processing (5)
src/app/actions/messages.tssrc/app/golf/actions/golf.tssrc/app/golf/actions/tasks.tssrc/lib/notifications/push.tssrc/test/lib/notifications/push.test.ts
…thout-rls anchor pattern Both files only ALTER an existing table; the rule matched the literal 'CREATE TABLE IF NOT EXISTS public.<name>' quoted in their history notes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
…+ roster wall (mobile split × row menu) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Owner directive: exhaustive feature sweep for "details and flows we might've missed... but also make sure the feature works." A 23-agent flow trace (every control → action → write → revalidate → consuming surface, plus prod error-table mining) surfaced 148 candidates; 47 survived adversarial confirmation (28 P0 / 19 P1; 1 refuted of 50 checked).
Fix / Outcome
17 fixer packets (each adversarially verified, repair loops until clean) + lead hand-repairs:
require_coach_approvaldefaults true on 10/10 prod teams; joiners previously vanished into pending with no UI anywhere).event_id), recurring events now invite attendees + notify on create/edit/delete, task-reminder cron actually matches user-set reminders.DB
Four replay-safe migrations applied to prod and verified (pg_policy / information_schema / proacl): settings-audit reconcile + capability-scoped INSERT policy, decision-log reconcile, practice-blocks visibility RLS,
mark_golf_messages_readRPC (anon revoked).Residual (tracked)
savePartialRoundsame-course/same-day collision narrowed but not eliminated — full fix is client-generated round ids (follow-up).Verification
tsc ✓ · eslint ✓ · full vitest suite 19,641 tests / 1,949 files ✓ · production build ✓ · contract tripwires updated with the new truth (registry 419→421, stat-layer grandfather, observability wraps).
Git Activity Timeline note
Feature-flow sweep: 47 confirmed broken/stranded user flows fixed; 4 prod migrations; approve/reject joiners flow built.
🤖 Generated with Claude Code
https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX