docs(baseball): BaseballHelm bug audit (89 findings) + bulk issue-filing script - #424
Closed
njrini99-code wants to merge 20 commits into
Closed
docs(baseball): BaseballHelm bug audit (89 findings) + bulk issue-filing script#424njrini99-code wants to merge 20 commits into
njrini99-code wants to merge 20 commits into
Conversation
Address 17 open issues that had CodeRabbit implementation plans: RLS migration layout, semgrep rule fixes, revalidatePath audit, password reauth, college interests, import source guards, recruitable-college seed invariant, team join codes, command palette/nav registry, command center read model, atomic box-score RPC, product-trust contracts, Lighthouse preview skip path, and stats source-of-truth documentation. Closes #107, #108, #109, #110, #371, #379, #388, #395, #397, #398, #399, #400, #403, #407, #409, #412, #418 Co-authored-by: Cursor <cursoragent@cursor.com>
Add PASSWORD_CHANGE rate limit for changePasswordAction (fixes Vercel build). Derive coach context server-side in getStateCounts (#404). Harden migration replay for staff invitations, coach insights RLS, and lifting coaches schema. Co-authored-by: Cursor <cursoragent@cursor.com>
Disable automatic deployments for all branches except main so feature branches and open PRs no longer trigger Vercel preview deploys. Co-authored-by: Cursor <cursoragent@cursor.com>
Stop tracking append-only ultracode event/log files and ignore regenerated .cleanup/ and docs/operations/generated/ outputs so the working tree stays clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Gate public player profiles (#401), watchlist recruitability (#402), and showcase-only org nav (#367). Fix player calendar team resolution (#368), coach redirect from My Stats (#410), scout CSV HTTP status (#396), atomic invite redemption (#395), server-side import band recompute (#415), and college-player JUCO join guard. Closes #367, #368, #396, #401, #402, #410, #415 Refs #395 Co-authored-by: Cursor <cursoragent@cursor.com>
#416) Add requireBaseballPlayerRoute per CodeRabbit #410 plan. Block shell auth on server verification in useBaseballAuth (#416). Exclude inactive staff from active-context resolution (#405). Let authorized staff edit calendar events via capability check instead of creator-only (#369 partial). Co-authored-by: Cursor <cursoragent@cursor.com>
Harden seed-baseball-stats.mjs: remove committed secrets, dry-run default, production guard, deterministic upsert IDs. Add getRoster read model and server-driven roster page with explicit load/degraded states (#411). Fix import-source registry to honor disabled rows when multiple matches exist. Triggered @coderabbitai plan on #393 and #406. Co-authored-by: Cursor <cursoragent@cursor.com>
…369, #370, #378, #391, #408, #413) Canonical game create at /stats/games/create with /new redirect, performance nav/hub capability alignment, calendar can_manage_calendar guards, announcements load-error state, and baseball seed script secret hardening with CI tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate document write actions to withBaseballAction, issue signed storage URLs for preview/version history instead of public URLs, and guard revert with can_manage_documents. Co-authored-by: Cursor <cursoragent@cursor.com>
Stabilize import-source lookup and org-team cookie access; migrate remaining legacy baseball mutations to withBaseballAction; extend Rini demo seed and contract/e2e coverage for team-ops and stats surfaces. Co-authored-by: Cursor <cursoragent@cursor.com>
…d-issues # Conflicts: # .coderabbit.yaml Co-authored-by: njrini99-code <njrini99-code@users.noreply.github.com>
Co-authored-by: njrini99-code <njrini99-code@users.noreply.github.com>
Co-authored-by: njrini99-code <njrini99-code@users.noreply.github.com>
Co-authored-by: njrini99-code <njrini99-code@users.noreply.github.com>
…th contracts (#373, #377, #384, #394) - #394: migrate interests/onboarding/philosophy/recruiting-philosophy/ watchlist actions to withBaseballAction; team identity is now always derived from the validated active context, not caller-supplied teamId. - #373: add scripts/route-crawler-baseball.mjs — authenticated coach session crawler over nav-registry-derived baseball routes, wired with a contract test. - #377/#384: add src/contracts/baseball/coachhelm-product-truth.contract.test.ts pinning source_refs citation, coachhelm action outcome-seeding, outcome- sweep improvement-sign usage, and stats-center read-model authorization envelopes. Points at the BaseballHelm generator (coachhelm/baseball/ generators/composites.ts), not the GolfHelm v2 orchestrator. - Fix documents/page.tsx to use the real ActiveBaseballContext shape (activeTeamId/activeRole/activeCoachId) and the real ReadModelStateNotice props (state, not message/loadState); use EmptyState for the no-team case. - Fix two pre-existing test/impl mismatches blocking CI: - save-full-box-score: the action never reads ctx (team comes from the game row), so skip active-context resolution via requireActiveContext: false; the test mock was also missing .maybeSingle() for the capability-resolution queries it now needs. - team-join-code: the success-path assertion still expected a stale-read-then-update on used_count; the implementation already redeems atomically via try_redeem_baseball_team_invitation (#395). Updated the assertion and added a regression test for the release_baseball_team_invitation_redemption rollback path. - interests.test.ts: fix a queued mockResolvedValueOnce(null) leaking from one test into the next (vi.clearAllMocks doesn't drain the once queue) by resetting that specific mock before re-arming its default. Made with Cursor
…ling script 8 parallel codebase audits across recruiting/discovery, stats/performance, team operations, communication/content, player daily experience, coach variants/settings/onboarding, CoachHelm AI, and mobile UI/UX. Each finding is verified against on-disk source with file/line evidence and cross-checked against open issues #367-#418 to avoid duplicating tracked architectural work. gh CLI access in this environment is read-only (no issue-creation tool), so findings are packaged as a runnable script instead of being filed directly: scripts/baseballhelm-bug-audit/create-issues.mjs --yes
|
Too many files changed for review. ( Bypass the limit by tagging |
WalkthroughThis PR refactors the majority of baseball server actions onto a shared ChangesBaseballHelm refactor and hardening
Golf actions revalidation annotations and shot_type investigation
Sequence Diagram(s)sequenceDiagram
participant Client
participant processTeamInvitation
participant try_redeem_baseball_team_invitation
participant joinTeam
participant release_baseball_team_invitation_redemption
Client->>processTeamInvitation: redeem invite code
processTeamInvitation->>try_redeem_baseball_team_invitation: reserve use (RPC)
try_redeem_baseball_team_invitation-->>processTeamInvitation: true
processTeamInvitation->>joinTeam: attempt roster join
joinTeam-->>processTeamInvitation: failure
processTeamInvitation->>release_baseball_team_invitation_redemption: release reservation (RPC)
sequenceDiagram
participant Page as command-center/page.tsx
participant ResolveTeam as resolveCoachTeamIdWithCookie
participant ReadModel as getCommandCenter
participant Adapter as assembleCommandCenterClientProps
participant Client as CommandCenterClient
Page->>ResolveTeam: resolve active teamId
Page->>ReadModel: getCommandCenter(teamId, includeWeekEvents)
ReadModel-->>Page: CommandCenterReadModel
Page->>Adapter: assembleCommandCenterClientProps(team, model)
Adapter-->>Page: loadState, players, calendarEvents, summary
Page->>Client: render props
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related issues
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
ci
Verification pass confirmed all 7 re-checked wave-1 findings as STILL_VALID against current on-disk code. Added 4 new-area audits: video/lineups/lifting, scout-packets/decision-room/compare, roles-permissions/program-settings, coach-notes/AI-governance/academics. One near-duplicate (a second writeup of the join-code lookup bug) was caught during consolidation and dropped.
cursor
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 64
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/hooks/use-baseball-auth.ts (1)
52-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle profile query errors before deriving redirects.
Lines 52-72 read
userResult.data,coachResult.data, andplayerResult.datawithout checking*.error. A transient/RLS failure will look like “no role/profile” and can incorrectly clear the store and redirect an authenticated user to signup/onboarding. Check the three errors first and fail closed instead of interpreting errored reads as absent data.🤖 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/hooks/use-baseball-auth.ts` around lines 52 - 72, The profile lookup flow in use-baseball-auth.ts is treating errored Supabase reads as missing data, which can trigger the wrong redirect. In the auth check that reads userResult, coachResult, and playerResult, inspect each *.error before using *.data and deriving resolvedRole. If any query fails, fail closed by returning an auth error/blocked redirect instead of clearing state or sending the user to signup/onboarding. Keep the fix centered around the existing async lookup and role resolution logic in use-baseball-auth.src/components/baseball/dashboard-shell.tsx (1)
117-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the role-based fallback until nav context is loaded.
Lines 117-122 and 191 in
src/components/baseball/dashboard-shell.tsxpass missing capabilities as{}intoresolveActiveHub()andCommandPalette, even though Lines 95-111 already document thatnavContextcan be absent while the shell falls back to role-based nav. On those renders, capability-gated hub tabs and palette commands disappear for authorized coaches. Reuse the same fallback behavior here, or defer capability filtering until real capabilities arrive.Also applies to: 191-191
🤖 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/dashboard-shell.tsx` around lines 117 - 122, The dashboard shell is applying capability filtering too early when navContext is still missing, which causes authorized role-based hub tabs and command palette items to disappear. Update the resolveActiveHub and CommandPalette call sites in dashboard-shell.tsx to preserve the existing role-based fallback until navContext is loaded, rather than passing empty capabilities or filtering as if the user has none. Use the existing navContext checks and role-based navigation behavior already documented in the component to keep tabs and commands visible until real capabilities arrive.src/lib/baseball/nav-registry.ts (1)
683-692: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't hide
/dashboard/eventsfor non-showcase teams.This entry is still modeled here as a shared coach/player team-events surface, but
allowedProgramTypes: SHOWCASE_ORG_PROGRAM_TYPESplus the filter at Line 878 now removes it for everycollege,high_school, andjucocontext. That makes/baseball/dashboard/eventsunreachable from nav and ⌘K for most programs even though the route remains shared. Split the showcase-only org events view into a separate entry, or remove the program-type gate from this shared entry.Suggested fix
{ id: 'events', label: 'Events', href: '/baseball/dashboard/events', icon: IconFlag, @@ role: 'both', requiredCapability: null, - allowedProgramTypes: SHOWCASE_ORG_PROGRAM_TYPES, section: 'primary', },Also applies to: 878-883
🤖 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/nav-registry.ts` around lines 683 - 692, The shared `events` nav entry in `nav-registry.ts` is being filtered out for non-showcase programs because `allowedProgramTypes: SHOWCASE_ORG_PROGRAM_TYPES` is still applied and the registry filter at the `events` entry removes it from nav and ⌘K. Update the `events` item so the shared team-events surface stays visible for `college`, `high_school`, and `juco` contexts, either by removing the program-type gate from this entry or by splitting the showcase-only org events view into its own separate nav item while keeping the shared route unfiltered.src/app/baseball/(dashboard)/dashboard/travel/page.tsx (1)
112-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe new error UI is unreachable when team resolution itself fails.
src/app/baseball/(dashboard)/dashboard/travel/page.tsx:95-97setsloadError, butsrc/app/baseball/(dashboard)/dashboard/travel/page.tsx:112-122runs before the new error branch at:124-137. If any coach/player/team lookup throws beforesetTeamId(...)runs, the page still renders “No Team Found” instead of the retry notice.Suggested fix
- if (!teamId) { - return ( - <> - <Header title="Travel" subtitle="Team travel and expense tracking" /> - <div className="p-8 text-center"> - <h2 className="text-lg font-semibold text-warm-900 mb-2">No Team Found</h2> - <p className="text-warm-500">You must be on a team to access travel itineraries.</p> - </div> - </> - ); - } - if (loadError) { return ( <> <Header title="Travel" subtitle="Team travel and expense tracking" /> <div className="p-6 lg:p-8"> @@ </> ); } + + if (!teamId) { + return ( + <> + <Header title="Travel" subtitle="Team travel and expense tracking" /> + <div className="p-8 text-center"> + <h2 className="text-lg font-semibold text-warm-900 mb-2">No Team Found</h2> + <p className="text-warm-500">You must be on a team to access travel itineraries.</p> + </div> + </> + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/baseball/`(dashboard)/dashboard/travel/page.tsx around lines 112 - 137, The travel page is checking `!teamId` before `loadError`, so failures in `detectRoleAndLoad()` still fall through to the “No Team Found” UI instead of the retry notice. Update `TravelPage` to prioritize the `loadError` branch before the `teamId` fallback, and make sure `detectRoleAndLoad()`/`setLoadError` are the only path that renders `ReadModelStateNotice` when team resolution throws.
🤖 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 @.coderabbit/semgrep/helmv3.yml:
- Around line 250-254: The suppression in helmv3.yml is too broad because the
multiline pattern-not-regex clauses can span multiple function definitions and
accidentally hide a SECURITY DEFINER issue. Update the rule to scope the
exception to a single function body by using a function-scoped structural
suppression such as pattern-not-inside, tied to the SECURITY DEFINER and SET
search_path/ALTER FUNCTION constructs, so only the same function’s search_path
handling can suppress the finding.
In `@e2e/baseball-phase1.spec.ts`:
- Around line 150-152: The health check in the e2e route verification is too
generic because it only looks for error text and any visible heading, so
redirects like /baseball/login can still pass. Update the route checks around
page.goto in the baseball phase1 spec to assert the destination URL after each
navigation, using the page’s URL/expectation and at minimum confirming it is not
/baseball/login and matches the intended route before considering the page
healthy.
In `@scripts/__tests__/baseball-demo-seed-surfaces.test.mjs`:
- Around line 12-25: Update the baseball demo seed contract test to include the
acknowledgement surface seeded by scripts/seed-rini-baseball-demo.ts. Extend
REQUIRED_TABLES in baseball-demo-seed-surfaces.test.mjs with
baseball_announcement_acknowledgements so the test asserts that the
acknowledgement upsert remains present alongside the existing baseball_* tables.
In `@scripts/__tests__/scripts-no-committed-secrets.test.mjs`:
- Around line 12-23: The secret-scan test currently misses scripts like
seed-rini-baseball-demo.ts because listSeedScripts only collects files matching
BASEBALL_SEED_GLOB, so expand the matching logic to include that script (either
by broadening BASEBALL_SEED_GLOB or adding an explicit allowlist entry) while
keeping the existing recursive scan in listSeedScripts intact. Make sure the
updated filter still covers the baseball seed scripts already in scope and that
scripts/seed-rini-baseball-demo.ts is included in the checked set.
In `@scripts/baseballhelm-bug-audit/create-issues.mjs`:
- Around line 37-73: The bulk issue creation loop in create-issues.mjs is not
idempotent because it always calls gh issue create for every selected item, so
reruns can duplicate already-created issues. Update the createIssues flow around
the selected.entries() loop to first detect whether an issue already exists for
the current item (using a stable unique key from issue.title or another
persisted identifier) before invoking execFileSync, and skip creation when it
does. Keep the dry-run path unchanged, and make the retry behavior safe so
rerunning the script only creates missing issues.
In `@scripts/baseballhelm-bug-audit/issues.mjs`:
- Around line 576-578: The generated issue body currently includes
workspace-local “in-progress local diff” caveats that will become stale after
merge. Update the issue text in the affected issue-building logic so it uses a
durable PR/commit reference or a generic “re-check current HEAD before starting
work” phrasing instead of mentioning local on-disk diffs. Make the change in the
code that assembles the issue body in issues.mjs so the generated issues remain
actionable outside the current workspace.
In `@scripts/route-crawler-baseball.mjs`:
- Around line 17-25: Update route-crawler-baseball.mjs so env loading follows
the Node script rule by reading .env.local before process.env, then use the auth
flow to distinguish missing credentials from an actual failed login. In the
login helper that returns null today, make it return a clear success/failure
signal for the sign-in attempt, and in the main flow around the auth/session
handling and skip logic, only treat truly absent config as a benign skip while
exiting non-zero when /api/auth/login was attempted but no session cookie was
obtained. Use the existing BASE_URL, COACH, PLAYER, and the login/session
handling code path to keep the fix localized.
In `@scripts/seed-baseball-roster.mjs`:
- Around line 3-16: The seed script is reading only ambient env and is missing
Node/script setup, which triggers undefined globals in static analysis. Update
the top of the script to load environment variables from .env.local before any
env reads, then fall back to process.env for SUPABASE_URL,
SUPABASE_SERVICE_ROLE_KEY, BASEBALL_SEED_ORG_ID, and BASEBALL_SEED_COACH_ID.
Also mark the file as a Node script so process and console are recognized,
keeping the existing validation logic in place.
In `@scripts/seed-baseball-stats.mjs`:
- Around line 170-182: The seed flow in scripts/seed-baseball-stats.mjs only
upserts base player stats/aggregates, leaving game and box-score tables empty or
inconsistent. Update the seed logic around the session loop and related
aggregate writes to generate coherent baseball_games,
baseball_box_score_batting, baseball_box_score_pitching, and
baseball_player_season_stats rows first, then call
recalculate_baseball_season_stats, and only after that derive any legacy
compatibility rows from the same game set via the existing upsertRow path.
- Around line 15-69: Add a Node environment annotation at the top of the seed
script and change the env bootstrap so `parseConfig` reads from `.env.local`
first, then falls back to `process.env`, instead of relying on `dotenv/config`
alone. Update the startup flow around `parseConfig`, `getArg`, and the script’s
env access so `.env.local` is explicitly loaded before any config validation,
while keeping the existing `detId` and Supabase setup unchanged.
In `@src/app/baseball/`(dashboard)/dashboard/camps/page.tsx:
- Around line 380-409: The retry flow in the camps page is only refreshing the
camp list and is skipping the player-specific registration state rebuild. Update
the `onRetry` logic in `page.tsx` to mirror the initial player load path in the
camps page component by also fetching and restoring `registeredCamps` for the
`isPlayer && player` branch. Make sure the same player data-loading helper or
equivalent logic used in the initial load is reused here so the rendered action
buttons stay consistent after a retry.
In `@src/app/baseball/`(dashboard)/dashboard/documents/page.tsx:
- Around line 23-31: The documents page is treating every missing context as a
normal “join a team” empty state, even when getActiveBaseballContext() failed
due to an actual membership-resolution error. Update page.tsx to distinguish “no
active team/memberships” from a failed lookup returned by active-context.ts, and
render an error/retry state for the latter instead of EmptyState; use the
getActiveBaseballContext and EmptyState paths to locate the fallback logic.
In `@src/app/baseball/`(dashboard)/dashboard/stats/games/create/NewGameClient.tsx:
- Around line 167-173: The conditional time input in NewGameClient lacks an
accessible name, so add a visible label or an appropriate
aria-label/aria-labelledby to the time field rendered when “Also add to team
calendar” is enabled. Use the existing eventTime input in NewGameClient and keep
the current focus styling, ensuring the control remains keyboard and
screen-reader accessible for the calendar-event flow.
- Around line 16-18: The default game date in NewGameClient should use the
user’s local calendar date instead of UTC, since the current toISOString()
behavior can preselect the wrong day for users in negative time zones. Update
the today value in NewGameClient (and the gameDate state initialization that
uses it) to derive YYYY-MM-DD from local date fields or by applying
getTimezoneOffset() before serialization.
- Around line 69-80: The toggle buttons in NewGameClient only indicate the
active game type/home-away choice through styling, so expose the selected state
to assistive tech by adding aria-pressed on each option or converting the pair
to a proper radio-group pattern. Update the interactive controls around the
gameType and homeAway selectors so screen readers can identify which choice is
active, while keeping their existing Button handling and visible focus behavior
intact.
In `@src/app/baseball/`(dashboard)/dashboard/stats/games/create/page.tsx:
- Around line 11-29: This page is resolving the team incorrectly and silently
treating Supabase failures as missing data, which causes bad redirects. Update
the create page to use the canonical shared server team resolver/guard instead
of the organization-scoped `.single()` lookup in the team fetch block, and keep
the page aligned with the navigation/team-resolution contract. In both the coach
and team fetches, check for and throw on actual Supabase query errors rather
than redirecting on null results alone, using the existing `supabase` calls and
the page’s team-resolution flow to locate the fix.
In `@src/app/baseball/`(public)/player/[id]/page.tsx:
- Around line 36-37: The page title in the player profile metadata is
concatenating a nullable player.primary_position, which can render “null” in
shared/SEO-indexed titles. Update the title generation in the page.tsx metadata
logic to guard player.primary_position and fall back to a safe label or omit the
position when it is unset, while keeping access.displayName as-is.
In `@src/app/baseball/actions/academics.ts`:
- Around line 361-369: Remove the redundant exported catch logging in Baseball
Academics so control-flow failures are not re-emitted to Sentry: in the
try/catch around updateEligibilityAction, let withBaseballAction handle logging
and only keep the error-to-domain mapping via mapAcademicsActionError. Apply the
same fix to the related exported action handlers in this refactor, such as
createEligibilityRecord and upsertPlayerAcademics, and remove any direct
logServerError calls from these outer catches.
- Around line 424-428: The `error instanceof z.ZodError` branches in
`createEligibilityRecordAction` and `upsertAcademicsAction` are unreachable
because `withBaseballAction` converts thrown validation errors into
`BaseballActionError`, which then gets mapped to a generic message. Move the
`eligibilitySchema.parse()` and `upsertAcademicsSchema.parse()` validation
outside the `withBaseballAction` body, or return validation failures directly
from the body, so the field-specific Zod message is preserved and the dead
`ZodError` checks can be removed.
- Around line 395-401: `updateEligibilityAction` is spreading unvalidated client
`data` directly into the Supabase update, which allows arbitrary column writes;
fix this by validating the input with the same zod-based pattern used in
`createEligibilityRecordAction` and `upsertPlayerAcademicsAction` before any DB
call. In `updateEligibilityAction`, replace the raw spread with a whitelist of
allowed updatable fields from the parsed payload and keep `updated_by` set from
`ctx.user.id` only. Do not accept or forward client-supplied IDs or ownership
fields such as `team_id` or `player_id`; only pass server-approved fields to
`.update()`.
In `@src/app/baseball/actions/auth.ts`:
- Around line 470-551: The changePasswordAction flow is manually checking raw
password strings instead of validating inputs with Zod first. Add a small Zod
schema near changePasswordAction to parse and trim currentPassword and
newPassword before any Supabase call, then use the parsed values for the
existing branching and password checks. Keep the rest of the logic in
changePasswordAction the same, but ensure all required/empty validation lives in
the schema so server and client validation stay aligned.
In `@src/app/baseball/actions/dev-plans.ts`:
- Around line 238-253: The denial and not-found paths in completeGoalAction and
uncompleteGoalAction are throwing plain Error, which gets treated as an
unexpected failure by withBaseballAction and converted into a generic
BaseballActionError. Update the ownership check to throw a passthrough error
type that mapDevPlanCoachError recognizes, and adjust the Goal not found /
fetchError paths to use a distinct non-generic signal so the user-facing message
is preserved and Sentry is not triggered for expected denials.
In `@src/app/baseball/actions/documents.ts`:
- Around line 690-700: Add the auth gate before any database access in the
`getPreviewUrl` path: call `supabase.auth.getUser()` before the first `.from()`
query against `baseball_document_versions`, and apply the same check in
`getVersionHistory` since it has the same pattern. If the user is not
authenticated, fail early before minting a signed URL or relying on RLS, and
keep the existing query logic unchanged after the gate.
In `@src/app/baseball/actions/games.ts`:
- Around line 692-700: The recalc step in the game completion flow is swallowing
RPC failures because `db2.rpc('recalculate_baseball_season_stats', ...)` returns
a result object instead of throwing, so `Promise.all` in the completion logic
never checks `.error`. Update the recalculation block in the game completion
action to collect each RPC result, inspect `error` for every player, and log
failures with enough context (playerId, teamId, season year) while keeping the
successful results unchanged. Use the existing completion path around the
`recalculate_baseball_season_stats` call to ensure stale season stats are
surfaced instead of silently ignored.
In `@src/app/baseball/actions/interests.ts`:
- Around line 78-127: The add-to-interests flow in interests.ts writes a
recruiting interest for any active player, but it must be gated by
recruitability first. Before the insert in addToInterests, use the shared
recruitability check or verify baseball_players.player_type and
recruiting_activated for ctx.activePlayerId, and reject college players or
non-opted-in players with a BaseballActionError. Keep the existing organization
and duplicate-interest checks, but ensure the recruitability validation happens
before logSecurityEvent and fromUntyped(...).insert.
In `@src/app/baseball/actions/lineups.ts`:
- Around line 118-127: The catch in saveLineup (and the same pattern in the
other affected action handlers) is re-logging wrapper/expected Baseball errors
as unexpected server errors. Update the catch logic to skip logServerError for
BaseballUnauthorizedError, BaseballNoActiveTeamError, BaseballCapabilityError,
and BaseballActionError, and only log truly unexpected failures; then still
return the mapped action error via mapLineupActionError or the corresponding
mapper.
- Around line 46-61: validateLineupPositions currently only checks duplicates
and count, but the lineup actions still persist client-supplied playerId values
without proving they belong to the target team. Add zod validation in the lineup
creation/update flow (including the lineup input schema used by the action
handlers) to enforce UUIDs and batting order range 1..9, then before any
insert/update in the lineup persistence path query baseball_team_members for all
submitted player IDs tied to the relevant teamId/lineup.team_id. If any IDs are
missing or inactive, reject the request and ensure the validated IDs are the
only ones passed into the Supabase write in the lineup action functions.
- Around line 83-107: The lineup save flow in the `saveLineup`/replacement path
is not atomic: the code creates or deletes `baseball_team_lineups` and
`baseball_lineup_positions` in separate steps, so a failure can leave an empty
or partially saved lineup. Refactor the write path used around the lineup
creation and replacement logic into a single atomic server-side operation, such
as a transactional RPC or stored procedure, so both the parent lineup and its
positions are committed or rolled back together. Update the code in `saveLineup`
and the replacement block that clears and reinserts positions to use that atomic
path instead of DELETE-then-INSERT cleanup.
In `@src/app/baseball/actions/onboarding.ts`:
- Around line 220-229: The exported completeCoachOnboarding action currently
accepts _preVerifiedUser and can bypass the withBaseballAction session check
before runCompleteCoachOnboardingCore performs admin writes. Remove that bypass
from the public action, and move the preverified flow into a non-exported helper
such as completeCoachOnboardingForVerifiedUser that is only called by
signupAndCompleteCoachOnboarding. Keep completeCoachOnboarding always verifying
the session before any DB access, and use the internal helper only for
already-verified users.
- Around line 461-472: The onboarding flow currently sets recruiting_active
based on playerType in the signup path, which wrongly opts in every non-college
player; update the onboarding logic in the player creation flow to always
persist recruiting_activated as false by default. Keep the
CommonSchemas.recruitingPlayerState.parse validation in place, but ensure the
insert into baseball_players in the onboarding action uses the opt-in model and
leaves activation to the explicit recruiting activation flow later.
In `@src/app/baseball/actions/recruiting-philosophy.ts`:
- Around line 147-162: Return sanitized database errors instead of raw Supabase
messages in the recruiting philosophy actions. In the `update`, `insert`, and
`delete` paths inside `recruiting-philosophy.ts` (including
`resetRecruitingPhilosophyAction`), replace direct uses of
`updateError.message`, `insertError.message`, and `deleteError.message` with
`sanitizeDbError(...)` so the client never receives schema, constraint, or RLS
details. Use the existing `sanitizeDbError` helper already used by
`getRecruitingPhilosophy`, `getPlayerPercentiles`, and `recalculatePercentiles`.
In `@src/app/baseball/actions/stat-event-imports.ts`:
- Around line 408-437: The commit flow in stat-event-imports should not rely on
client-supplied args.rows after server-side detection from rawFileBody. In the
main import path around detectFromParse and the later commit step, either
replace the committed rows with the server-parsed parsed.rows when rawFileBody
is present, or explicitly compare parsed.rows against args.rows and reject on
mismatch. Make sure the existing autoCommitBand / requiresReview gating in this
action is applied to the same server-verified payload that gets persisted.
In `@src/app/baseball/actions/tasks.ts`:
- Around line 682-688: CreateTaskTemplateAction, deleteTaskTemplateAction, and
updateTaskTemplateAction mutate template data but return success without
refreshing the templates page, so add revalidation using TASK_TEMPLATES_PATH
before each success return. Keep the existing mutation logic, and ensure
createTaskTemplateAction, deleteTaskTemplateAction, and updateTaskTemplateAction
all call revalidatePath() after the change and before returning their success
payload; seedDefaultTemplates already shows the expected pattern.
In `@src/app/baseball/actions/teams.ts`:
- Around line 703-755: The getCoachTeamForManagement action is still using the
default active-context requirement, so it can fail before the null-return path
when ctx.activeTeamId is missing or stale. Update the withBaseballAction call
for getCoachTeamForManagement to set requireActiveContext: false, then make the
body safely handle an empty ctx.activeTeamId before querying baseball_teams so
the action can return success with null instead of throwing.
- Around line 646-699: `regenerateTeamInviteCode` is missing the coach-type
restriction and currently only checks organization ownership. Update the
`regenerateTeamInviteCode` action in `teams.ts` to apply the same coach-type
guard used by `generateTeamInviteCode` and `getCoachTeamForManagement`, blocking
`high_school` and `showcase` coaches from rotating join codes. Keep the existing
organization check, but add the coach-type validation before calling
`persistUniqueJoinCode`, and return the same kind of unauthorized/error response
used by the other team-management paths.
- Around line 764-768: The join-code lookup in joinTeamByCode is happening
before any auth verification, which lets anonymous callers probe baseball_teams
and violates the server-action contract. Move the supabase.auth.getUser() check
to the start of joinTeamByCode, or route this flow through processTeamInvitation
so authentication and ownership checks happen before any
fromUntyped(...).select(...) call. Keep the TeamByCode lookup only after the
user is confirmed.
In `@src/app/baseball/actions/travel.ts`:
- Around line 158-180: The outer try/catch in createItinerary is treating
validation errors as generic failures because createItinerarySchema.parse runs
inside createItineraryAction, so by the time control returns the error is no
longer a ZodError. Move the Zod validation handling into createItineraryAction
itself (or validate before calling withBaseballAction) so field-level messages
are returned directly instead of being converted to the generic travel action
error; apply the same fix pattern to updateItineraryAction and addExpenseAction,
where the same parse-vs-catch mismatch exists.
In `@src/app/baseball/actions/watchlist.ts`:
- Around line 246-251: The `mapWatchlistActionError` `z.ZodError` path is
unreachable because `WatchlistSchemas.updateStatus.parse(...)` (and the same
parse pattern in `updateWatchlistPriorityAction` and `addWatchlistNoteAction`)
runs inside the `withBaseballAction` wrapper, which converts validation failures
into `BaseballActionError` before the outer mapper sees them. Move validation so
the action returns or throws the validation result directly in
`updateWatchlistStatusAction`/related actions, or pre-validate in the exported
wrapper, and make sure `mapWatchlistActionError` is only used for errors it can
actually receive so invalid input surfaces with the real schema message instead
of the generic fallback.
In `@src/components/baseball/ReadModelStateNotice.tsx`:
- Around line 37-49: Add live-region semantics to the notice rendered by
ReadModelStateNotice so async failure/authorization states are announced to
assistive tech. Update the Card or its top-level wrapper in ReadModelStateNotice
to use an appropriate alert/live region pattern (such as role="alert" with
aria-live) while preserving the existing state-specific content and retry button
behavior.
In `@src/contracts/baseball/product-trust.contract.test.ts`:
- Around line 8-53: The product-trust contract tests are only checking for
source-text tokens, so they can pass on comments or dead code instead of
validating behavior. Update the tests in the baseball contract suite to import
the real modules/components (for example CommandCenterClient, DailyBriefPanel,
StatsCenterClient, and the read-model helpers) and assert on exported values or
rendered UI states rather than grep-based strings. Keep the same coverage for
authorized/error/empty cases, but make the assertions user-visible and tied to
the actual contract surface.
In `@src/hooks/use-baseball-auth.ts`:
- Around line 107-128: The Zustand profile update logic in use-baseball-auth is
reusing stale store state after store.clear(), which can leak fields from a
previous user into the newly verified coach/player profile. Update the profile
reconciliation block to avoid spreading store.coach and store.player from the
pre-clear snapshot when cachedCoachId or cachedPlayerId differ from freshCoachId
or freshPlayerId; instead re-read the store after clear() or only merge existing
fields when the cached and fresh IDs still match.
In `@src/lib/baseball/__tests__/active-context-staff-status.test.ts`:
- Around line 3-14: The test duplicates the staff-status filtering logic instead
of exercising the production implementation, so it won’t catch regressions in
loadMemberships or the revoked-status check. Move the status predicate out of
active-context.ts into a shared exported helper such as isRevokedStaffStatus (or
the shared constant/module referenced there), then import that helper into
active-context-staff-status.test.ts and assert its behavior directly. Keep the
revoked-status literals centralized so this test and active-context.ts use the
same source of truth.
In `@src/lib/baseball/__tests__/resolve-team.test.ts`:
- Around line 150-157: `validateCoachTeamAccess` only has a positive coverage
case right now; add a negative test in `resolve-team.test.ts` for a coach staff
record whose `baseball_team_coach_staff.status` is `removed` or `suspended`, and
assert the function returns false. Use the existing `makeSupabaseClient` and
`validateCoachTeamAccess` symbols in the new test so the staff-status
authorization guard stays covered against regressions.
In `@src/lib/baseball/__tests__/server-route-guards.test.ts`:
- Around line 22-24: The test is mocking `@/lib/supabase/server` inline with a
bare createClient stub instead of using the shared canonical Supabase test
client. Update server-route-guards.test.ts to import and use the test client
helper from src/test/ so the mock behavior stays consistent across tests, and
remove the ad-hoc vi.fn() mock for createClient. Keep the test wired through the
shared Supabase test utilities rather than a local replacement.
In `@src/lib/baseball/active-context.ts`:
- Line 86: The revoked-status list is duplicated in active-context logic and can
drift from the canonical source in capabilities.ts. Export REVOKED_STATUSES from
src/lib/baseball/capabilities.ts, then import and use it in active-context.ts
wherever the inline suspended/removed/invited check appears so the
activeTeamId/activeRole resolution stays aligned. Also update
src/lib/baseball/__tests__/active-context-staff-status.test.ts to reference the
same shared constant instead of hardcoding the status array, using the existing
active-context and REVOKED_STATUSES symbols to locate the affected checks.
In `@src/lib/baseball/import-source-enabled.ts`:
- Around line 75-79: The import-source registry lookup in import-source-enabled
ignores the query error and can fall back to adapter defaults, so update the
db.from('baseball_import_sources').select(...).eq('team_id', teamId) call to
capture both data and error, then immediately throw when the lookup fails before
any filtering or defaulting logic runs. Use the import-source-enabled flow and
the registry lookup variables in that function to keep the fix localized and
ensure failed reads fail closed.
In `@src/lib/baseball/public-profile-access.ts`:
- Around line 35-41: Update public-profile access in public-profile-access so
only active relationships count: in the staff check that uses
supabase.from('baseball_team_coach_staff') and the membership lookups later in
the file, filter out removed/suspended/inactive rows instead of accepting any
historical record. Make the access decision in the relevant helper functions and
the public profile fetch path use only current active staff/membership records,
and ensure the program-level public profile setting is selected from the active
team relationship rather than an arbitrary past team.
In `@src/lib/baseball/read-models/command-center-adapter.ts`:
- Around line 53-57: The load state mapping in getCommandCenter is collapsing
any non-null model.error into a full error state, which loses the existing
partial state from resolveReadModelLoadState. Update the call so it can
distinguish degraded-but-usable responses by preserving partial when
roster/events/feed data are still present, using the existing
resolveReadModelLoadState and getCommandCenter logic to keep safe arrays
available while only showing full error when appropriate.
In `@src/lib/baseball/read-models/command-center.ts`:
- Around line 375-377: The aggregate lookup in command-center’s
rosterPulse/rosterPlayers flow is choosing the first baseball_player_aggregates
row instead of the row for the current team, which can surface another team’s
data. Update the aggregate selection logic in the relevant mapping block to
filter by the current team’s team_id before using it, and apply the same fix in
the other affected rosterPlayers section so both paths use the correct
team-scoped aggregate row.
- Around line 238-248: The week range in command-center.ts is being anchored to
server time instead of the requested date, so align the `includeWeekEvents`
window with the already-normalized `day` value. Update the week-start/week-end
calculation inside `command-center` to derive from `day` in UTC rather than
creating a fresh `new Date()`, keeping the `todayEvents` and `weekEvents`
queries on the same timeline. Use the existing `forDate`/`day` flow and preserve
the current `weekStartIso` and `weekEndIso` outputs.
In `@src/lib/baseball/read-models/roster.ts`:
- Around line 128-135: The roster query is bypassing type safety with a
`supabase as any` cast in the `baseball_player_aggregates` fetch. Replace that
escape hatch in `roster.ts` with the existing untyped helper pattern used
elsewhere for Supabase reads, and keep the result typed via explicit
`BaseballPlayerAggregates` types rather than suppressing `no-explicit-any`.
In `@src/lib/baseball/recruitability.ts`:
- Around line 3-4: Move CoachType out of the action layer by removing the
dependency from recruitability.ts on src/app/baseball/actions/discover.
Re-export CoachType from a shared domain location such as `@/lib/types` or another
baseball domain module, then update the import in the recruitability helper to
use that shared type source instead. Keep the change limited to the type
definition and its import path, so src/lib/baseball/recruitability.ts no longer
reaches into the app actions layer.
- Around line 39-44: The discoverability lookup in recruitability is using all
rows from baseball_team_members, so inactive or removed players can still be
treated as discoverable. Update the query in the recruitability helper to
restrict results to active roster memberships only, using the existing
membership fields and any status/active indicator already used elsewhere in the
baseball membership flow. Keep the Set-building logic in the recruitability
function, but ensure it only includes currently active player_id values from the
filtered membership records.
In `@src/lib/baseball/resolve-team-server.ts`:
- Around line 2-3: The import in resolve-team-server should avoid a deep
reference to the generated database type and instead use the type barrel. Update
the Database type import in resolve-team-server to come from `@/lib/types` so the
module stays decoupled from the generated file layout and aligns with the
re-exports centralized in src/lib/types/index.ts.
In `@src/lib/baseball/resolve-team.ts`:
- Around line 28-35: Exclude non-active staff records from team resolution and
team listing. Update the staff lookup in resolveTeam and the shared
team-selection/listing logic to filter out revoked states such as removed,
suspended, and invited instead of treating any matching
baseball_team_coach_staff row as valid. Use the existing resolveTeam function
and the active-team query/list code around the staff-row handling to add the
status check consistently in both places.
- Around line 1-2: The import in resolve-team should not deep-import the
generated Database type from the database module; update the Database import to
come from the project barrel at `@/lib/types` instead. Keep the existing
SupabaseClient import unchanged, and verify resolve-team continues to use the
Database symbol through the re-exported types entrypoint.
In `@src/lib/baseball/with-baseball-action.ts`:
- Around line 323-334: The fallback context in withBaseballAction is fabricating
a coach-like ActiveBaseballContext when requireActiveContext is false, which can
mislead later checks on ctx.context.activeRole or ctx.activeRole. Update
withBaseballAction so the inactive/no-context path does not default to
activeRole: 'coach' or an empty activeTeamId; instead make context and
activeRole nullable/absent in that branch, or introduce a narrower first-write
context that only exposes user and targetTeamId while leaving active fields
undefined.
In `@src/lib/product-trust/read-model-state.ts`:
- Around line 33-39: The resolveReadModelLoadState function is checking
!input.hasData before input.partial, which causes partial:true with no data to
incorrectly return empty instead of partial. Update the evaluation order in
resolveReadModelLoadState so the partial flag is handled before the !hasData
branch, preserving the documented contract for ReadModelLoadState. Add a
regression test covering the authorized, no-error, hasData:false, partial:true
case to ensure it resolves to partial.
In `@supabase/investigations/shot_type_investigation.sql`:
- Around line 1-26: Add an explicit read-only transaction around the
investigation queries in shot_type_investigation.sql so the script self-enforces
the no DDL/DML requirement instead of relying only on the operator’s role. Keep
the existing SELECTs and the constraint check intact, and make the transaction
boundaries clear at the top and bottom of the script.
In `@supabase/migrations/20260624000050_baseball_rls_helpers_and_policies.sql`:
- Around line 570-572: The INSERT policy on baseball_event_acknowledgements only
validates user_id via auth.uid(), so it must also restrict event_id to events
the user is allowed to access. Update the policy creation in the
baseball_event_acks_insert rule to add an EXISTS check against baseball_events
that confirms the authenticated user is either on the event’s team or a coach
for that team, using the existing baseball_events access patterns/helpers to
keep the RLS boundary enforced in the database.
In `@supabase/migrations/20260630000000_baseball_save_full_box_score_rpc.sql`:
- Around line 56-58: The save path in baseball_save_full_box_score_rpc still
uses DELETE-then-INSERT semantics for the per-game box score tables, so update
the RPC to keep canonical rows and replace them with keyed upserts instead. Use
the existing save function/transaction flow around p_game_id to switch the
batting and pitching writes to ON CONFLICT-based upserts, then remove only truly
stale rows after all replacements succeed, or add a per-game lock if needed to
serialize the full save.
- Around line 67-69: The RPC in the baseball save box-score function trusts
`player_id` values from `p_batting` and `p_pitching` and writes them with
`team_id = v_team_id`, so add a pre-insert validation step in the save flow to
confirm every payload player exists in `baseball_team_members` for `v_team_id`
before any `baseball_box_score_batting` or `baseball_box_score_pitching`
insertions. Use the existing function context around `v_team_id`, `p_batting`,
and `p_pitching` to collect all referenced player IDs, reject the entire call on
any mismatch, and only proceed with canonical stat persistence
(`baseball_games`, `baseball_box_score_batting`, `baseball_box_score_pitching`)
when all players belong to the team.
In `@supabase/migrations/20260630180200_baseball_team_invitation_redeem_rpc.sql`:
- Around line 12-18: The invitation redemption flow in the redeem/release RPCs
is not caller-owned, so any authenticated user can release a redemption for any
invitation and reopen exhausted invites. Update the
`baseball_team_invitation_redeem_rpc` and
`release_baseball_team_invitation_redemption` flow so redemption is tied to the
caller, either by merging reservation plus team join into one atomic RPC or by
storing a redemption record keyed by `(invitation_id, auth.uid())` and requiring
that ownership on release. Make sure the rollback path used by `teams.ts` after
`joinTeam()` fails only releases the caller’s own reservation.
In `@vercel.json`:
- Around line 7-10: The deploymentEnabled setting is disabling all non-main
branches, which prevents preview deployments needed by the CircleCI preview
flow. Update the Vercel configuration to allow preview builds for PR/feature
branches instead of using a blanket "*" false rule, and apply branch filtering
or an Ignored Build Step so the deployment logic still works. Adjust the
relevant vercel.json deployment rules so main remains enabled without blocking
preview URLs.
---
Outside diff comments:
In `@src/app/baseball/`(dashboard)/dashboard/travel/page.tsx:
- Around line 112-137: The travel page is checking `!teamId` before `loadError`,
so failures in `detectRoleAndLoad()` still fall through to the “No Team Found”
UI instead of the retry notice. Update `TravelPage` to prioritize the
`loadError` branch before the `teamId` fallback, and make sure
`detectRoleAndLoad()`/`setLoadError` are the only path that renders
`ReadModelStateNotice` when team resolution throws.
In `@src/components/baseball/dashboard-shell.tsx`:
- Around line 117-122: The dashboard shell is applying capability filtering too
early when navContext is still missing, which causes authorized role-based hub
tabs and command palette items to disappear. Update the resolveActiveHub and
CommandPalette call sites in dashboard-shell.tsx to preserve the existing
role-based fallback until navContext is loaded, rather than passing empty
capabilities or filtering as if the user has none. Use the existing navContext
checks and role-based navigation behavior already documented in the component to
keep tabs and commands visible until real capabilities arrive.
In `@src/hooks/use-baseball-auth.ts`:
- Around line 52-72: The profile lookup flow in use-baseball-auth.ts is treating
errored Supabase reads as missing data, which can trigger the wrong redirect. In
the auth check that reads userResult, coachResult, and playerResult, inspect
each *.error before using *.data and deriving resolvedRole. If any query fails,
fail closed by returning an auth error/blocked redirect instead of clearing
state or sending the user to signup/onboarding. Keep the fix centered around the
existing async lookup and role resolution logic in use-baseball-auth.
In `@src/lib/baseball/nav-registry.ts`:
- Around line 683-692: The shared `events` nav entry in `nav-registry.ts` is
being filtered out for non-showcase programs because `allowedProgramTypes:
SHOWCASE_ORG_PROGRAM_TYPES` is still applied and the registry filter at the
`events` entry removes it from nav and ⌘K. Update the `events` item so the
shared team-events surface stays visible for `college`, `high_school`, and
`juco` contexts, either by removing the program-type gate from this entry or by
splitting the showcase-only org events view into its own separate nav item while
keeping the shared route unfiltered.
🪄 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: 5d91d952-ecf6-4663-9950-835c5a39f4a9
⛔ Files ignored due to path filters (2)
.ultracode/baseballhelm/cc-4878.logis excluded by!**/*.log.ultracode/baseballhelm/cc.logis excluded by!**/*.log
📒 Files selected for processing (147)
.circleci/README.md.circleci/config.yml.circleci/scripts/wait-for-vercel-preview.sh.coderabbit/semgrep/helmv3.yml.gitignore.ultracode/baseballhelm/events.ndjsondocs/BASEBALLHELM_BUG_AUDIT_2026-06-30.mddocs/operations/BASEBALL_STATS_SOURCE_OF_TRUTH.mde2e/baseball-box-score.spec.tse2e/baseball-phase1.spec.tsscripts/__tests__/baseball-action-integrity.test.mjsscripts/__tests__/baseball-demo-seed-surfaces.test.mjsscripts/__tests__/baseball-stale-route-links.test.mjsscripts/__tests__/route-crawler-baseball.test.mjsscripts/__tests__/scripts-no-committed-secrets.test.mjsscripts/__tests__/seed-baseball-stats.safety.test.mjsscripts/__tests__/seed-recruiting-invariant.test.mjsscripts/baseballhelm-bug-audit/create-issues.mjsscripts/baseballhelm-bug-audit/issues.mjsscripts/route-crawler-baseball.mjsscripts/seed-baseball-demo.tsscripts/seed-baseball-roster.mjsscripts/seed-baseball-stats.mjsscripts/seed-rini-baseball-demo.tssrc/app/baseball/(dashboard)/_components/hub-definitions.tssrc/app/baseball/(dashboard)/_components/hub-sub-nav.tsxsrc/app/baseball/(dashboard)/_components/resolve-active-hub.tssrc/app/baseball/(dashboard)/dashboard/announcements/page.tsxsrc/app/baseball/(dashboard)/dashboard/calendar/page.tsxsrc/app/baseball/(dashboard)/dashboard/camps/page.tsxsrc/app/baseball/(dashboard)/dashboard/command-center/page.tsxsrc/app/baseball/(dashboard)/dashboard/compare/CompareClient.tsxsrc/app/baseball/(dashboard)/dashboard/dev-plan/page.tsxsrc/app/baseball/(dashboard)/dashboard/discover/DiscoverClient.tsxsrc/app/baseball/(dashboard)/dashboard/documents/page.tsxsrc/app/baseball/(dashboard)/dashboard/import/page.tsxsrc/app/baseball/(dashboard)/dashboard/my-stats/page.tsxsrc/app/baseball/(dashboard)/dashboard/roster/RosterClient.tsxsrc/app/baseball/(dashboard)/dashboard/roster/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/NewGameClient.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/error.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/loading.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/new/page.tsxsrc/app/baseball/(dashboard)/dashboard/travel/page.tsxsrc/app/baseball/(public)/packet/[token]/csv/route.tssrc/app/baseball/(public)/player/[id]/page.tsxsrc/app/baseball/actions/__tests__/change-password.test.tssrc/app/baseball/actions/__tests__/interests.test.tssrc/app/baseball/actions/__tests__/save-full-box-score.test.tssrc/app/baseball/actions/__tests__/team-join-code.test.tssrc/app/baseball/actions/academics.tssrc/app/baseball/actions/announcements.tssrc/app/baseball/actions/auth.tssrc/app/baseball/actions/calendar.tssrc/app/baseball/actions/camps.tssrc/app/baseball/actions/dev-plans.tssrc/app/baseball/actions/discover.tssrc/app/baseball/actions/documents.tssrc/app/baseball/actions/games.tssrc/app/baseball/actions/imports.tssrc/app/baseball/actions/interests.tssrc/app/baseball/actions/lineups.tssrc/app/baseball/actions/onboarding.tssrc/app/baseball/actions/philosophy.tssrc/app/baseball/actions/player-peek.tssrc/app/baseball/actions/recruiting-philosophy.tssrc/app/baseball/actions/stat-event-imports.tssrc/app/baseball/actions/tasks.tssrc/app/baseball/actions/teams.tssrc/app/baseball/actions/travel.tssrc/app/baseball/actions/watchlist.tssrc/app/golf/actions/demo-access.tssrc/app/golf/actions/insight-celebration.tssrc/app/golf/actions/message-attachments.tssrc/app/golf/actions/push-notifications.tssrc/app/golf/actions/round-recap.tssrc/app/golf/actions/v3/focus-area-progress.tssrc/app/golf/actions/v3/goal-progress.tssrc/components/CommandPalette.tsxsrc/components/baseball/ReadModelStateNotice.tsxsrc/components/baseball/command-center/CommandCenterClient.tsxsrc/components/baseball/dashboard-shell.tsxsrc/components/baseball/games/GamesList.tsxsrc/components/features/college-card.tsxsrc/contracts/baseball/coachhelm-product-truth.contract.test.tssrc/contracts/baseball/demo-stats-smoke.contract.test.tssrc/contracts/baseball/product-trust.contract.test.tssrc/contracts/baseball/route-shell.contract.test.tssrc/hooks/use-baseball-auth.tssrc/hooks/use-dashboard.tssrc/lib/auth/supabase-rate-limit.tssrc/lib/baseball/__tests__/active-context-staff-status.test.tssrc/lib/baseball/__tests__/command-palette-nav.test.tssrc/lib/baseball/__tests__/documents-capability.test.tssrc/lib/baseball/__tests__/import-source-enabled.test.tssrc/lib/baseball/__tests__/import-source-registration.test.tssrc/lib/baseball/__tests__/nav-performance-capability.test.tssrc/lib/baseball/__tests__/resolve-active-hub.test.tssrc/lib/baseball/__tests__/resolve-team.test.tssrc/lib/baseball/__tests__/roster-read-model.test.tssrc/lib/baseball/__tests__/server-route-guards.test.tssrc/lib/baseball/__tests__/showcase-org-nav.test.tssrc/lib/baseball/__tests__/stats-route-aliases.test.tssrc/lib/baseball/active-context.tssrc/lib/baseball/capabilities.tssrc/lib/baseball/capability-groups.tssrc/lib/baseball/command-palette-nav.tssrc/lib/baseball/import-source-enabled.tssrc/lib/baseball/nav-registry.tssrc/lib/baseball/public-profile-access.tssrc/lib/baseball/read-models/__tests__/command-center.test.tssrc/lib/baseball/read-models/command-center-adapter.tssrc/lib/baseball/read-models/command-center.tssrc/lib/baseball/read-models/roster.tssrc/lib/baseball/recruitability.tssrc/lib/baseball/resolve-team-server.tssrc/lib/baseball/resolve-team.tssrc/lib/baseball/server-route-guards.tssrc/lib/baseball/stats-route-aliases.tssrc/lib/baseball/with-baseball-action.tssrc/lib/db-error.tssrc/lib/product-trust/__tests__/read-model-state.test.tssrc/lib/product-trust/read-model-state.tssrc/lib/types/baseball-staff-roles.tssrc/lib/validation/action-schemas.tssrc/lib/validation/server-action-validator.tssupabase/investigations/shot_type_findings.mdsupabase/investigations/shot_type_investigation.sqlsupabase/migrations/20260624000050_baseball_rls_helpers_and_policies.sqlsupabase/migrations/20260624000061_baseball_lifting_performance.sqlsupabase/migrations/20260624000063_baseball_v11_premium_lifting.sqlsupabase/migrations/20260624000080_baseball_elite_stat_event_model.sqlsupabase/migrations/20260624000090_baseball_settings_os.sqlsupabase/migrations/20260624000092_baseball_signals_and_actions.sqlsupabase/migrations/20260624000093_baseball_postgame_reviews.sqlsupabase/migrations/20260624000094_baseball_practice_effectiveness.sqlsupabase/migrations/20260624000221_baseball_video_links_and_class_conflicts.sqlsupabase/migrations/20260624000230_baseball_signal_action_materialization.sqlsupabase/migrations/20260624000450_baseball_ai_audit_log.sqlsupabase/migrations/20260630000000_baseball_save_full_box_score_rpc.sqlsupabase/migrations/20260630170000_baseball_college_recruiting_check.sqlsupabase/migrations/20260630180000_baseball_scope_player_ids_rls.sqlsupabase/migrations/20260630180100_baseball_documents_capability.sqlsupabase/migrations/20260630180200_baseball_team_invitation_redeem_rpc.sqlvercel.json
Comment on lines
250
to
+254
| patterns: | ||
| - pattern: SECURITY DEFINER | ||
| - pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,500}set\\s+search_path" | ||
| - pattern-regex: "(?i)security\\s+definer" | ||
| - pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,8000}set\\s+search_path" | ||
| - pattern-not-regex: "(?is)alter\\s+function[\\s\\S]{0,2000}set\\s+search_path" | ||
| - pattern-not-regex: "(?is)alter\\s+function[\\s'\\s%]*set\\s+search_path" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the changed rule block and nearby context.
file=".coderabbit/semgrep/helmv3.yml"
echo "== File exists? =="
ls -l "$file"
echo
echo "== Line count =="
wc -l "$file"
echo
echo "== Relevant excerpt =="
sed -n '220,270p' "$file" | cat -nRepository: njrini99-code/helmv3
Length of output: 2114
🌐 Web query:
Semgrep generic mode pattern-not-regex file scope semantics pattern-regex pattern-not-regex multiple matches same file
💡 Result:
In Semgrep's generic mode, the interpretation of rules relies on a simplified document structure of tokens, indentation, and newlines, rather than a deep language-specific parser [1]. The behavior of filtering operators like pattern-not-regex and pattern-not in this mode, especially regarding file scope and multiple matches, is defined by specific matching semantics [1][2]. 1. pattern-not-regex Semantics: This operator filters findings that have any overlap with the provided PCRE2 regular expression [2][3]. It operates at the file level in multiline mode [2]. A critical nuance is that pattern-not-regex filters out a match if it intersects with the range of the regex match [4][5]. However, users should be aware that if the regex does not correctly encompass the entire span of the intended exclusion (for example, missing end-of-line characters or boundaries), the finding may not be filtered as expected [6][5]. 2. pattern-not Semantics: Unlike pattern-not-regex, the pattern-not operator is strictly structural [4]. It filters out a finding only if the pattern-not clause matches exactly the same code span as the primary pattern [7][4]. This makes it less effective for broad filtering compared to pattern-not-regex when the target code varies in structure or size [7][4]. 3. Generic Mode Specifics: - File Scope: In generic mode, the document is interpreted as a sequence of ASCII words and punctuation [1]. Indentation determines nesting, and braces are only treated as structural within single lines [1]. - Ellipsis (...): The ellipsis operator spans up to 10 lines by default, but this limit is configurable via generic_ellipsis_max_span [1][2]. - Multiple Matches in the Same File: Semgrep evaluates patterns against the entire file content. Because generic mode treats the document as a stream of tokens rather than a structured tree, multiple matches for the same pattern are processed independently [1]. If multiple matches exist, each finding is evaluated against the exclusion logic (pattern-not or pattern-not-regex) individually [4]. For robust exclusion in generic mode, prefer pattern-not-regex when you need to filter based on textual patterns that overlap with the match [2][4]. If the filtering requires structural awareness (e.g., ensuring code is not inside a specific block), use pattern-not-inside [7]. If you encounter issues where expected exclusions are not occurring, verify that your regex accounts for the full context of the match, including line endings, as generic mode does not automatically handle all language-specific boundary nuances [6][5].
Citations:
- 1: https://semgrep.dev/docs/writing-rules/generic-pattern-matching
- 2: https://semgrep.dev/docs/writing-rules/rule-syntax
- 3: https://docs.semgrep.dev/writing-rules/rule-syntax
- 4: Using pattern-inside with pattern-not not working as expected. semgrep/semgrep#8428
- 5: Trying to understand pattern-not-regex's exclusion behavior semgrep/semgrep#11025
- 6: pattern-not-regex does not filter as expected in generic mode semgrep/semgrep#5148
- 7: https://semgrep.dev/docs/kb/rules/using-pattern-not-inside
🌐 Web query:
Semgrep pattern-not-regex excludes finding if regex matches anywhere in file or only overlapping the primary match official docs
💡 Result:
According to official Semgrep documentation, the pattern-not-regex operator filters out findings that have any overlap with the provided PCRE2 regular expression [1][2]. It does not filter based on matches anywhere in the file unless those matches overlap with the original finding's range [3][4]. When Semgrep identifies a potential finding (the "match"), it uses the pattern-not-regex clause to check for an intersection between that match's range and the range identified by the regex [4]. If the regex match overlaps with any part of the finding's span, the finding is excluded [1][3][2]. Key points regarding this behavior include: 1. Scope: The exclusion is tied to the specific finding [3][4]. It is not a global exclusion that removes all instances of a pattern from the entire file, but rather a filter that removes specific findings that meet the exclusion criteria [1][2]. 2. Overlap: The exclusion triggers when there is any overlap between the ranges of the positive match and the negative regex [3][4]. 3. Complexity: Because this works on ranges, if your regular expressions do not precisely align with the expected tokens or line boundaries, findings may not be excluded as expected [5][6]. If a regex is intended to match a whole line but does not account for specific character sequences or anchors (like ^ or $), the overlap might not occur as intended, leading to unexpected results [5][6]. In summary, pattern-not-regex works by filtering specific findings where the exclusion regex overlaps with the range of the initial match [1][3][4].
Citations:
- 1: https://semgrep.dev/docs/writing-rules/rule-syntax
- 2: https://docs.semgrep.dev/writing-rules/rule-syntax
- 3: Change pattern-not-regex to filter when regex overlaps with a match semgrep/semgrep#2572
- 4: pattern-not-regex does not properly filter out matches semgrep/semgrep#2465
- 5: Trying to understand pattern-not-regex's exclusion behavior semgrep/semgrep#11025
- 6: pattern-not-regex does not filter as expected in generic mode semgrep/semgrep#5148
Scope the suppression to one function body. .coderabbit/semgrep/helmv3.yml:250-254 uses multiline pattern-not-regex clauses that can bridge separate definitions, so a later SET search_path / ALTER FUNCTION ... SET search_path can hide an earlier SECURITY DEFINER finding in the same migration. Use a function-scoped pattern-not-inside/structural match instead of broad regex suppression.
🤖 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 @.coderabbit/semgrep/helmv3.yml around lines 250 - 254, The suppression in
helmv3.yml is too broad because the multiline pattern-not-regex clauses can span
multiple function definitions and accidentally hide a SECURITY DEFINER issue.
Update the rule to scope the exception to a single function body by using a
function-scoped structural suppression such as pattern-not-inside, tied to the
SECURITY DEFINER and SET search_path/ALTER FUNCTION constructs, so only the same
function’s search_path handling can suppress the finding.
Comment on lines
+150
to
+152
| await page.goto(route); | ||
| await expect(page.locator('body')).not.toContainText(/Application error|500/i); | ||
| await expect(page.locator('h1, h2').first()).toBeVisible({ timeout: 10000 }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the destination route before treating the page as healthy.
Line 150 only checks for generic error text plus any visible h1/h2. If one of these routes redirects to /baseball/login or another fallback page, this test still passes. Add a URL assertion after each goto (at minimum, "not /baseball/login") so the test actually verifies coach access to the intended surface.
🤖 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 `@e2e/baseball-phase1.spec.ts` around lines 150 - 152, The health check in the
e2e route verification is too generic because it only looks for error text and
any visible heading, so redirects like /baseball/login can still pass. Update
the route checks around page.goto in the baseball phase1 spec to assert the
destination URL after each navigation, using the page’s URL/expectation and at
minimum confirming it is not /baseball/login and matches the intended route
before considering the page healthy.
Comment on lines
+12
to
+25
| const REQUIRED_TABLES = [ | ||
| 'baseball_announcements', | ||
| 'baseball_travel_itineraries', | ||
| 'baseball_travel_expenses', | ||
| 'baseball_tasks', | ||
| 'baseball_task_assignments', | ||
| ]; | ||
|
|
||
| test('rini baseball demo seed covers team-ops surfaces', async () => { | ||
| const text = await readFile(SEED_FILE, 'utf8'); | ||
| for (const table of REQUIRED_TABLES) { | ||
| assert.match(text, new RegExp(`upsert\\('${table}'`)); | ||
| } | ||
| assert.match(text, /can_manage_documents:\s*true/); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Cover the acknowledgement seed in this contract.
REQUIRED_TABLES omits baseball_announcement_acknowledgements, even though scripts/seed-rini-baseball-demo.ts, Lines 296-301 now seed that row. If the read-receipt seed disappears, this test still passes while the “Read receipts required” announcement path regresses.
Suggested fix
const REQUIRED_TABLES = [
'baseball_announcements',
+ 'baseball_announcement_acknowledgements',
'baseball_travel_itineraries',
'baseball_travel_expenses',
'baseball_tasks',
'baseball_task_assignments',
];📝 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 REQUIRED_TABLES = [ | |
| 'baseball_announcements', | |
| 'baseball_travel_itineraries', | |
| 'baseball_travel_expenses', | |
| 'baseball_tasks', | |
| 'baseball_task_assignments', | |
| ]; | |
| test('rini baseball demo seed covers team-ops surfaces', async () => { | |
| const text = await readFile(SEED_FILE, 'utf8'); | |
| for (const table of REQUIRED_TABLES) { | |
| assert.match(text, new RegExp(`upsert\\('${table}'`)); | |
| } | |
| assert.match(text, /can_manage_documents:\s*true/); | |
| const REQUIRED_TABLES = [ | |
| 'baseball_announcements', | |
| 'baseball_announcement_acknowledgements', | |
| 'baseball_travel_itineraries', | |
| 'baseball_travel_expenses', | |
| 'baseball_tasks', | |
| 'baseball_task_assignments', | |
| ]; | |
| test('rini baseball demo seed covers team-ops surfaces', async () => { | |
| const text = await readFile(SEED_FILE, 'utf8'); | |
| for (const table of REQUIRED_TABLES) { | |
| assert.match(text, new RegExp(`upsert\\('${table}'`)); | |
| } | |
| assert.match(text, /can_manage_documents:\s*true/); |
🤖 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 `@scripts/__tests__/baseball-demo-seed-surfaces.test.mjs` around lines 12 - 25,
Update the baseball demo seed contract test to include the acknowledgement
surface seeded by scripts/seed-rini-baseball-demo.ts. Extend REQUIRED_TABLES in
baseball-demo-seed-surfaces.test.mjs with baseball_announcement_acknowledgements
so the test asserts that the acknowledgement upsert remains present alongside
the existing baseball_* tables.
Comment on lines
+12
to
+23
| /** Baseball seed / roster scripts in scope for #391 (incremental hardening). */ | ||
| const BASEBALL_SEED_GLOB = /^seed-baseball-.*\.(mjs|ts)$/; | ||
|
|
||
| async function listSeedScripts(dir, acc = []) { | ||
| const entries = await readdir(dir, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| const full = join(dir, entry.name); | ||
| if (entry.isDirectory()) { | ||
| if (entry.name === '__tests__' || entry.name === 'node_modules') continue; | ||
| await listSeedScripts(full, acc); | ||
| } else if (BASEBALL_SEED_GLOB.test(entry.name)) { | ||
| acc.push(full); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Include seed-rini-baseball-demo.ts in the secret scan.
BASEBALL_SEED_GLOB only matches seed-baseball-*, so scripts/seed-rini-baseball-demo.ts is never checked. A hardcoded service-role JWT or prod Supabase URL there would bypass this guard entirely.
Suggested fix
-const BASEBALL_SEED_GLOB = /^seed-baseball-.*\.(mjs|ts)$/;
+const BASEBALL_SEED_GLOB = /^seed-(?:baseball|rini-baseball)-.*\.(mjs|ts)$/;📝 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
| /** Baseball seed / roster scripts in scope for #391 (incremental hardening). */ | |
| const BASEBALL_SEED_GLOB = /^seed-baseball-.*\.(mjs|ts)$/; | |
| async function listSeedScripts(dir, acc = []) { | |
| const entries = await readdir(dir, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const full = join(dir, entry.name); | |
| if (entry.isDirectory()) { | |
| if (entry.name === '__tests__' || entry.name === 'node_modules') continue; | |
| await listSeedScripts(full, acc); | |
| } else if (BASEBALL_SEED_GLOB.test(entry.name)) { | |
| acc.push(full); | |
| /** Baseball seed / roster scripts in scope for `#391` (incremental hardening). */ | |
| const BASEBALL_SEED_GLOB = /^seed-(?:baseball|rini-baseball)-.*\.(mjs|ts)$/; | |
| async function listSeedScripts(dir, acc = []) { | |
| const entries = await readdir(dir, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const full = join(dir, entry.name); | |
| if (entry.isDirectory()) { | |
| if (entry.name === '__tests__' || entry.name === 'node_modules') continue; | |
| await listSeedScripts(full, acc); | |
| } else if (BASEBALL_SEED_GLOB.test(entry.name)) { | |
| acc.push(full); |
🤖 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 `@scripts/__tests__/scripts-no-committed-secrets.test.mjs` around lines 12 -
23, The secret-scan test currently misses scripts like
seed-rini-baseball-demo.ts because listSeedScripts only collects files matching
BASEBALL_SEED_GLOB, so expand the matching logic to include that script (either
by broadening BASEBALL_SEED_GLOB or adding an explicit allowlist entry) while
keeping the existing recursive scan in listSeedScripts intact. Make sure the
updated filter still covers the baseball seed scripts already in scope and that
scripts/seed-rini-baseball-demo.ts is included in the checked set.
Comment on lines
+37
to
+73
| const selected = issues.slice(startIdx - 1, endIdx); | ||
|
|
||
| console.log( | ||
| `${isDryRun ? '[DRY RUN] ' : ''}Preparing to file ${selected.length} of ${issues.length} BaseballHelm bug-audit issues (range ${startIdx}-${endIdx}).\n` | ||
| ); | ||
|
|
||
| let created = 0; | ||
| let failed = 0; | ||
|
|
||
| for (const [i, issue] of selected.entries()) { | ||
| const num = startIdx + i; | ||
| console.log(`[${num}/${issues.length}] ${issue.title}`); | ||
|
|
||
| if (isDryRun) { | ||
| console.log(` labels: ${issue.labels.join(', ')}`); | ||
| continue; | ||
| } | ||
|
|
||
| const tmpFile = join(tmpdir(), `baseballhelm-issue-${num}-${Date.now()}.md`); | ||
| writeFileSync(tmpFile, issue.body, 'utf8'); | ||
|
|
||
| try { | ||
| const output = execFileSync( | ||
| 'gh', | ||
| [ | ||
| 'issue', | ||
| 'create', | ||
| '--title', | ||
| issue.title, | ||
| '--body-file', | ||
| tmpFile, | ||
| '--label', | ||
| issue.labels.join(','), | ||
| ], | ||
| { encoding: 'utf8' } | ||
| ); | ||
| console.log(` -> ${output.trim()}`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the bulk issue filer idempotent.
Line 59 always calls gh issue create, so rerunning the script after a partial failure or an operator retry will file duplicate issues for anything already created. That violates the repo rule for one-off ops scripts and will corrupt the GitHub issue set this tool is supposed to generate.
As per path instructions, "One-off ops scripts. Must be idempotent (re-running is safe)."
Suggested guard
for (const [i, issue] of selected.entries()) {
const num = startIdx + i;
console.log(`[${num}/${issues.length}] ${issue.title}`);
if (isDryRun) {
console.log(` labels: ${issue.labels.join(', ')}`);
continue;
}
+
+ const existing = JSON.parse(
+ execFileSync(
+ 'gh',
+ ['issue', 'list', '--state', 'all', '--search', `in:title "${issue.title}"`, '--json', 'title'],
+ { encoding: 'utf8' }
+ )
+ );
+ if (existing.some((row) => row.title === issue.title)) {
+ console.log(' -> already exists, skipping');
+ continue;
+ }
const tmpFile = join(tmpdir(), `baseballhelm-issue-${num}-${Date.now()}.md`);📝 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 selected = issues.slice(startIdx - 1, endIdx); | |
| console.log( | |
| `${isDryRun ? '[DRY RUN] ' : ''}Preparing to file ${selected.length} of ${issues.length} BaseballHelm bug-audit issues (range ${startIdx}-${endIdx}).\n` | |
| ); | |
| let created = 0; | |
| let failed = 0; | |
| for (const [i, issue] of selected.entries()) { | |
| const num = startIdx + i; | |
| console.log(`[${num}/${issues.length}] ${issue.title}`); | |
| if (isDryRun) { | |
| console.log(` labels: ${issue.labels.join(', ')}`); | |
| continue; | |
| } | |
| const tmpFile = join(tmpdir(), `baseballhelm-issue-${num}-${Date.now()}.md`); | |
| writeFileSync(tmpFile, issue.body, 'utf8'); | |
| try { | |
| const output = execFileSync( | |
| 'gh', | |
| [ | |
| 'issue', | |
| 'create', | |
| '--title', | |
| issue.title, | |
| '--body-file', | |
| tmpFile, | |
| '--label', | |
| issue.labels.join(','), | |
| ], | |
| { encoding: 'utf8' } | |
| ); | |
| console.log(` -> ${output.trim()}`); | |
| const selected = issues.slice(startIdx - 1, endIdx); | |
| console.log( | |
| `${isDryRun ? '[DRY RUN] ' : ''}Preparing to file ${selected.length} of ${issues.length} BaseballHelm bug-audit issues (range ${startIdx}-${endIdx}).\n` | |
| ); | |
| let created = 0; | |
| let failed = 0; | |
| for (const [i, issue] of selected.entries()) { | |
| const num = startIdx + i; | |
| console.log(`[${num}/${issues.length}] ${issue.title}`); | |
| if (isDryRun) { | |
| console.log(` labels: ${issue.labels.join(', ')}`); | |
| continue; | |
| } | |
| const existing = JSON.parse( | |
| execFileSync( | |
| 'gh', | |
| ['issue', 'list', '--state', 'all', '--search', `in:title "${issue.title}"`, '--json', 'title'], | |
| { encoding: 'utf8' } | |
| ) | |
| ); | |
| if (existing.some((row) => row.title === issue.title)) { | |
| console.log(' -> already exists, skipping'); | |
| continue; | |
| } | |
| const tmpFile = join(tmpdir(), `baseballhelm-issue-${num}-${Date.now()}.md`); | |
| writeFileSync(tmpFile, issue.body, 'utf8'); | |
| try { | |
| const output = execFileSync( | |
| 'gh', | |
| [ | |
| 'issue', | |
| 'create', | |
| '--title', | |
| issue.title, | |
| '--body-file', | |
| tmpFile, | |
| '--label', | |
| issue.labels.join(','), | |
| ], | |
| { encoding: 'utf8' } | |
| ); | |
| console.log(` -> ${output.trim()}`); |
🧰 Tools
🪛 ESLint
[error] 39-39: 'console' is not defined.
(no-undef)
[error] 48-48: 'console' is not defined.
(no-undef)
[error] 51-51: 'console' is not defined.
(no-undef)
[error] 73-73: 'console' is not defined.
(no-undef)
🤖 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 `@scripts/baseballhelm-bug-audit/create-issues.mjs` around lines 37 - 73, The
bulk issue creation loop in create-issues.mjs is not idempotent because it
always calls gh issue create for every selected item, so reruns can duplicate
already-created issues. Update the createIssues flow around the
selected.entries() loop to first detect whether an issue already exists for the
current item (using a stable unique key from issue.title or another persisted
identifier) before invoking execFileSync, and skip creation when it does. Keep
the dry-run path unchanged, and make the retry behavior safe so rerunning the
script only creates missing issues.
Source: Path instructions
Comment on lines
+570
to
572
| EXECUTE $p$CREATE POLICY "baseball_event_acks_insert" ON public.baseball_event_acknowledgements | ||
| FOR INSERT TO authenticated | ||
| WITH CHECK (user_id = auth.uid())$p$; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Constrain acknowledgement inserts to events the user can access.
Line 572 only checks user_id = auth.uid(), so any authenticated user with an event_id can insert an acknowledgement for another team’s event and then read it via the owner branch. Add an EXISTS check against baseball_events requiring the user to be on that team or a coach for that team.
Suggested policy shape
EXECUTE $p$CREATE POLICY "baseball_event_acks_insert" ON public.baseball_event_acknowledgements
FOR INSERT TO authenticated
- WITH CHECK (user_id = auth.uid())$p$;
+ WITH CHECK (
+ user_id = auth.uid()
+ AND EXISTS (
+ SELECT 1
+ FROM public.baseball_events e
+ WHERE e.id = baseball_event_acknowledgements.event_id
+ AND (
+ public.is_baseball_team_coach_v2(e.team_id)
+ OR public.is_baseball_team_player(e.team_id)
+ )
+ )
+ )$p$;As per path instructions, RLS policies must enforce the database access boundary rather than trusting client-side filtering.
📝 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
| EXECUTE $p$CREATE POLICY "baseball_event_acks_insert" ON public.baseball_event_acknowledgements | |
| FOR INSERT TO authenticated | |
| WITH CHECK (user_id = auth.uid())$p$; | |
| EXECUTE $p$CREATE POLICY "baseball_event_acks_insert" ON public.baseball_event_acknowledgements | |
| FOR INSERT TO authenticated | |
| WITH CHECK ( | |
| user_id = auth.uid() | |
| AND EXISTS ( | |
| SELECT 1 | |
| FROM public.baseball_events e | |
| WHERE e.id = baseball_event_acknowledgements.event_id | |
| AND ( | |
| public.is_baseball_team_coach_v2(e.team_id) | |
| OR public.is_baseball_team_player(e.team_id) | |
| ) | |
| ) | |
| )$p$; |
🤖 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/20260624000050_baseball_rls_helpers_and_policies.sql`
around lines 570 - 572, The INSERT policy on baseball_event_acknowledgements
only validates user_id via auth.uid(), so it must also restrict event_id to
events the user is allowed to access. Update the policy creation in the
baseball_event_acks_insert rule to add an EXISTS check against baseball_events
that confirms the authenticated user is either on the event’s team or a coach
for that team, using the existing baseball_events access patterns/helpers to
keep the RLS boundary enforced in the database.
Source: Path instructions
Comment on lines
+56
to
+58
| DELETE FROM public.baseball_box_score_batting WHERE game_id = p_game_id; | ||
| DELETE FROM public.baseball_box_score_pitching WHERE game_id = p_game_id; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Replace the DELETE-then-INSERT save path.
This is a full box-score save/submit path, but it deletes canonical rows before rebuilding them. Use keyed upserts (ON CONFLICT) plus a targeted stale-row prune after successful upserts, or take a per-game transaction lock and preserve rows until replacements are guaranteed.
As per path instructions, “Forbid bulk DELETE-then-INSERT in functions/triggers — use UPSERT/ON CONFLICT.”
🤖 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/20260630000000_baseball_save_full_box_score_rpc.sql`
around lines 56 - 58, The save path in baseball_save_full_box_score_rpc still
uses DELETE-then-INSERT semantics for the per-game box score tables, so update
the RPC to keep canonical rows and replace them with keyed upserts instead. Use
the existing save function/transaction flow around p_game_id to switch the
batting and pitching writes to ON CONFLICT-based upserts, then remove only truly
stale rows after all replacements succeed, or add a per-game lock if needed to
serialize the full save.
Source: Path instructions
Comment on lines
+67
to
+69
| p_game_id, | ||
| (v_bat->>'player_id')::uuid, | ||
| v_team_id, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Validate every payload player belongs to the game team before inserting canonical stats.
The RPC only verifies the coach can access v_team_id; it then trusts player_id from p_batting / p_pitching and stamps team_id = v_team_id. Because this SECURITY DEFINER function bypasses normal RLS, a crafted payload can attach another team’s player to this game and corrupt box-score + season stats.
Validate all payload player_ids against baseball_team_members for v_team_id before Lines 56-58, and reject the whole save on any mismatch. Based on learnings, baseball_games, baseball_box_score_batting, and baseball_box_score_pitching are the canonical persisted stats source of truth.
Also applies to: 102-104
🤖 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/20260630000000_baseball_save_full_box_score_rpc.sql`
around lines 67 - 69, The RPC in the baseball save box-score function trusts
`player_id` values from `p_batting` and `p_pitching` and writes them with
`team_id = v_team_id`, so add a pre-insert validation step in the save flow to
confirm every payload player exists in `baseball_team_members` for `v_team_id`
before any `baseball_box_score_batting` or `baseball_box_score_pitching`
insertions. Use the existing function context around `v_team_id`, `p_batting`,
and `p_pitching` to collect all referenced player IDs, reject the entire call on
any mismatch, and only proceed with canonical stat persistence
(`baseball_games`, `baseball_box_score_batting`, `baseball_box_score_pitching`)
when all players belong to the team.
Source: Learnings
Comment on lines
+12
to
+18
| UPDATE public.baseball_team_invitations | ||
| SET used_count = COALESCE(used_count, 0) + 1 | ||
| WHERE id = p_invitation_id | ||
| AND is_active = true | ||
| AND (expires_at IS NULL OR expires_at > now()) | ||
| AND (max_uses IS NULL OR COALESCE(used_count, 0) < max_uses) | ||
| RETURNING id INTO v_id; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Bind invitation redemption and release to a caller-owned reservation.
release_baseball_team_invitation_redemption lets any authenticated caller decrement used_count for any invitation UUID. That can reopen exhausted invitations or undo other users’ legitimate redemptions. The current two-RPC flow also leaves membership creation outside the same atomic unit.
Move reservation + team join into one RPC, or persist a redemption row keyed by (invitation_id, auth.uid()) and require that key when releasing. The downstream teams.ts flow relies on this rollback path after joinTeam() fails, so the rollback needs ownership semantics.
Also applies to: 27-41
🤖 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/20260630180200_baseball_team_invitation_redeem_rpc.sql`
around lines 12 - 18, The invitation redemption flow in the redeem/release RPCs
is not caller-owned, so any authenticated user can release a redemption for any
invitation and reopen exhausted invites. Update the
`baseball_team_invitation_redeem_rpc` and
`release_baseball_team_invitation_redemption` flow so redemption is tied to the
caller, either by merging reservation plus team join into one atomic RPC or by
storing a redemption record keyed by `(invitation_id, auth.uid())` and requiring
that ownership on release. Make sure the rollback path used by `teams.ts` after
`joinTeam()` fails only releases the caller’s own reservation.
Comment on lines
+7
to
+10
| "deploymentEnabled": { | ||
| "*": false, | ||
| "main": true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## vercel.json\n'
git ls-files vercel.json vercel.ts .circleci/config.yml .circleci/README.md | xargs -r -I{} sh -c 'echo "--- {}"; cat -n "{}" | sed -n "1,220p"'
printf '\n## grep deploymentEnabled / lighthouse-preview\n'
rg -n '"deploymentEnabled"|lighthouse-preview|preview deployment|preview URL|deploymentEnabled' vercel.json .circleci/config.yml .circleci/README.mdRepository: njrini99-code/helmv3
Length of output: 2114
🌐 Web query:
Vercel git.deploymentEnabled branch-scoped behavior preview deployments disabled all branches except main
💡 Result:
To restrict deployments in Vercel so that only the main branch triggers them, you cannot use a simple "allowlist" in the git.deploymentEnabled configuration, as that field is designed primarily to explicitly disable specific branches or patterns [1]. Because Vercel defaults to enabling deployments for any unspecified branch, you must use an Ignored Build Step script to enforce an allowlist effectively [1][2][3]. Recommended Approach: Ignored Build Step Using an Ignored Build Step is the most reliable way to ensure only specific branches (e.g., main) trigger deployments [2][3]. Go to your project's Git settings in the Vercel dashboard and add the following command as your Ignored Build Step [4][3]: if [[ "$VERCEL_GIT_COMMIT_REF" == "main" ]]; then # Proceed with the build (exit 1) echo "✅ - Build can proceed" exit 1; else # Don't build (exit 0) echo "🛑 - Build cancelled" exit 0; fi This script checks the VERCEL_GIT_COMMIT_REF environment variable. If the branch is main, it exits with 1 (proceeding with the build); otherwise, it exits with 0 (canceling the build) [2][3]. Understanding git.deploymentEnabled While you can use git.deploymentEnabled in your vercel.json, it acts as a denylist [1][5]. It accepts branch names or glob patterns (e.g., "feature-*") to explicitly disable deployments [1][6]. However, it does not support an "allow-only" logic where you could define all other branches as disabled by default [5][7]. If you use it, remember that the configuration file must be present in the branch being pushed for Vercel to recognize the rule [8]. Additional Control If your goal is simply to prevent preview deployments entirely, you can also manage this via the Vercel Dashboard under Project Settings > Environments > Preview by disabling branch tracking [9].
Citations:
- 1: https://vercel.com/docs/project-configuration/git-configuration
- 2: https://stackoverflow.com/questions/72586732/prevent-vercel-deployment-on-branch-push
- 3: Is there a way to disable auto deploy on specific branch? vercel/vercel#4307
- 4: https://saeidmohadjer.com/blog/disable-preview-deployment-vercel
- 5: https://community.vercel.com/t/allow-a-more-granular-selection-of-deployed-branches-to-an-environment/6284
- 6: https://vercel.com/changelog/granular-branch-matching-for-git-configuration-in-vercel-json
- 7: https://community.vercel.com/t/glob-patten-support-for-vercel-json-s-git-deploymentenabled-config/1856
- 8: deploymentEnabled option in vercel.json is ignored vercel/vercel#11176
- 9: https://community.vercel.com/t/how-to-disable-every-branch-deployment-on-preview-env-in-vercel/7210
🌐 Web query:
Vercel project configuration git.deploymentEnabled false main true wildcard object docs
💡 Result:
To configure automatic deployments for your Vercel project, you can use the git.deploymentEnabled property within your vercel.json or vercel.ts configuration file [1][2]. This property accepts either a boolean (to disable all deployments) or an object (to specify deployment rules per branch using exact names or glob patterns) [1][3]. Configuration Examples To disable automatic deployments for all branches, set the value to false: { "git": { "deploymentEnabled": false } } To define specific rules for branches, use an object where the key is the branch name (or a glob pattern) and the value is a boolean [1][3]: { "git": { "deploymentEnabled": { "main": true, "feature/": false, "internal-": false } } } Key Considerations 1. Branch Matching: Vercel supports glob patterns (e.g., internal-*) [3]. If a branch matches multiple rules and at least one evaluates to true, a deployment will be triggered [1]. 2. Default Behavior: By default, any branch not explicitly configured in the object is treated as true (enabled) [1]. 3. Deprecation Note: The older github.enabled property is deprecated; you should use git.deploymentEnabled for managing branch-based deployments [1][4]. For more complex scenarios where you need to check commit contents or other build-time conditions, consider using the Ignore Build Step feature instead of or in addition to git.deploymentEnabled [5][2][6].
Citations:
- 1: https://vercel.com/docs/project-configuration/git-configuration
- 2: https://vercel.com/docs/project-configuration
- 3: https://vercel.com/changelog/granular-branch-matching-for-git-configuration-in-vercel-json
- 4: https://github.com/marketplace/actions/vercel-action
- 5: https://community.vercel.com/t/glob-patten-support-for-vercel-json-s-git-deploymentenabled-config/1856
- 6: https://stackoverflow.com/questions/72586732/prevent-vercel-deployment-on-branch-push
🌐 Web query:
site:vercel.com/docs/project-configuration git.deploymentEnabled deploymentEnabled object main false true wildcard
💡 Result:
The git.deploymentEnabled property is a configuration setting used in Vercel projects (within vercel.json or vercel.ts) to control which Git branches trigger automatic deployments [1]. Key details include: Types and Structure: It can be set as a Boolean (to enable or disable all deployments) or as an Object where keys represent branch identifiers [1]. Default Behavior: By default, the value is true, meaning all branches trigger deployments [1]. Branch Control: When using an object, you can specify individual branches as true or false to enable or disable deployments for them specifically [1]. Wildcard Matching: You can use minimatch syntax within the object keys to match multiple branches (e.g., "internal-" to target all branches starting with that prefix) [1]. Example Usage: To disable deployments for a specific branch (e.g., dev) and all branches matching a pattern (e.g., internal-): { "git": { "deploymentEnabled": { "dev": false, "internal-*": false } } } If a branch matches multiple rules, it will trigger a deployment if at least one rule evaluates to true [1]. To turn off automatic deployments for all branches, simply set the property to false [1].
Citations:
🌐 Web query:
vercel deploymentEnabled "*" false "main" true preview deployments branch patterns denylist
💡 Result:
In Vercel projects, the git.deploymentEnabled property in vercel.json allows you to control which branches trigger automatic deployments [1]. You can use this setting to enable or disable deployments for specific branches or use glob patterns for more granular control [1][2]. Configuration Details: - The property accepts either a boolean or an object where keys represent branch patterns (using minimatch syntax) and values are booleans [1]. - By default, deployments are enabled for all branches (true) unless otherwise specified [1]. - To disable a specific branch (e.g., main), you would set its value to false [1]. - You can use glob patterns to match multiple branches, such as internal-* to disable deployments for all branches starting with internal- [1][2]. - If a branch matches multiple rules, a deployment will occur if at least one rule evaluates to true [1]. Important Considerations: - The vercel.json file must be present in the branch being pushed for these configurations to take effect [3]. - While you can disable deployments for specific branches, there is no native "allow-list only" mode (e.g., "disable all, only enable main") directly through this setting [4]. To achieve a similar effect, you would need to explicitly disable the unwanted branches or patterns [1][4]. - If you wish to turn off all automatic deployments for the entire project, you can set git.deploymentEnabled to false [1]. Example configuration: { "git": { "deploymentEnabled": { "main": true, "internal-*": false } } } [1]
Citations:
- 1: https://vercel.com/docs/project-configuration/git-configuration
- 2: https://vercel.com/changelog/granular-branch-matching-for-git-configuration-in-vercel-json
- 3: deploymentEnabled option in vercel.json is ignored vercel/vercel#11176
- 4: https://community.vercel.com/t/allow-a-more-granular-selection-of-deployed-branches-to-an-environment/6284
vercel.json:7-10 disables preview deployments for non-main branches. "*": false turns off every branch by default, and main: true only re-enables main; that removes PR/feature preview URLs and breaks the preview-dependent flow in .circleci/config.yml:230-249. Use an Ignored Build Step or preview branch filtering instead.
🤖 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 `@vercel.json` around lines 7 - 10, The deploymentEnabled setting is disabling
all non-main branches, which prevents preview deployments needed by the CircleCI
preview flow. Update the Vercel configuration to allow preview builds for
PR/feature branches instead of using a blanket "*" false rule, and apply branch
filtering or an Ignored Build Step so the deployment logic still works. Adjust
the relevant vercel.json deployment rules so main remains enabled without
blocking preview URLs.
Comment on lines
+380
to
+409
| onRetry={() => { | ||
| setLoading(true); | ||
| setLoadError(null); | ||
| void (async () => { | ||
| try { | ||
| if (isCoach && coach) { | ||
| const { data, error } = await supabase | ||
| .from('baseball_camps') | ||
| .select(`*, organization:organizations(id, name, logo_url), registrations:baseball_camp_registrations(count)`) | ||
| .eq('coach_id', coach.id) | ||
| .order('start_date', { ascending: true }); | ||
| if (error) throw error; | ||
| setCamps((data as Camp[]) || []); | ||
| } else if (isPlayer && player) { | ||
| const { data, error } = await supabase | ||
| .from('baseball_camps') | ||
| .select(`*, organization:organizations(id, name, logo_url), registrations:baseball_camp_registrations(count)`) | ||
| .eq('status', 'active') | ||
| .gte('end_date', new Date().toISOString()) | ||
| .order('start_date', { ascending: true }); | ||
| if (error) throw error; | ||
| setCamps((data as Camp[]) || []); | ||
| } | ||
| } catch { | ||
| setLoadError('Camps could not be loaded.'); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| })(); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retry path drops player registration state.
At src/app/baseball/(dashboard)/dashboard/camps/page.tsx:393-401, the player retry branch only reloads baseball_camps. The initial player load at src/app/baseball/(dashboard)/dashboard/camps/page.tsx:246-255 also rebuilds registeredCamps, so after a transient load failure the retry flow can render already-registered camps as unregistered and offer the wrong action.
Suggested fix
} else if (isPlayer && player) {
const { data, error } = await supabase
.from('baseball_camps')
.select(`*, organization:organizations(id, name, logo_url), registrations:baseball_camp_registrations(count)`)
.eq('status', 'active')
.gte('end_date', new Date().toISOString())
.order('start_date', { ascending: true });
if (error) throw error;
+ const { data: playerRegs } = await supabase
+ .from('baseball_camp_registrations')
+ .select('camp_id')
+ .eq('player_id', player.id)
+ .neq('status', 'cancelled');
+ setRegisteredCamps(new Set((playerRegs ?? []).map((r) => r.camp_id)));
setCamps((data as Camp[]) || []);
}📝 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
| onRetry={() => { | |
| setLoading(true); | |
| setLoadError(null); | |
| void (async () => { | |
| try { | |
| if (isCoach && coach) { | |
| const { data, error } = await supabase | |
| .from('baseball_camps') | |
| .select(`*, organization:organizations(id, name, logo_url), registrations:baseball_camp_registrations(count)`) | |
| .eq('coach_id', coach.id) | |
| .order('start_date', { ascending: true }); | |
| if (error) throw error; | |
| setCamps((data as Camp[]) || []); | |
| } else if (isPlayer && player) { | |
| const { data, error } = await supabase | |
| .from('baseball_camps') | |
| .select(`*, organization:organizations(id, name, logo_url), registrations:baseball_camp_registrations(count)`) | |
| .eq('status', 'active') | |
| .gte('end_date', new Date().toISOString()) | |
| .order('start_date', { ascending: true }); | |
| if (error) throw error; | |
| setCamps((data as Camp[]) || []); | |
| } | |
| } catch { | |
| setLoadError('Camps could not be loaded.'); | |
| } finally { | |
| setLoading(false); | |
| } | |
| })(); | |
| }} | |
| onRetry={() => { | |
| setLoading(true); | |
| setLoadError(null); | |
| void (async () => { | |
| try { | |
| if (isCoach && coach) { | |
| const { data, error } = await supabase | |
| .from('baseball_camps') | |
| .select(`*, organization:organizations(id, name, logo_url), registrations:baseball_camp_registrations(count)`) | |
| .eq('coach_id', coach.id) | |
| .order('start_date', { ascending: true }); | |
| if (error) throw error; | |
| setCamps((data as Camp[]) || []); | |
| } else if (isPlayer && player) { | |
| const { data, error } = await supabase | |
| .from('baseball_camps') | |
| .select(`*, organization:organizations(id, name, logo_url), registrations:baseball_camp_registrations(count)`) | |
| .eq('status', 'active') | |
| .gte('end_date', new Date().toISOString()) | |
| .order('start_date', { ascending: true }); | |
| if (error) throw error; | |
| const { data: playerRegs } = await supabase | |
| .from('baseball_camp_registrations') | |
| .select('camp_id') | |
| .eq('player_id', player.id) | |
| .neq('status', 'cancelled'); | |
| setRegisteredCamps(new Set((playerRegs ?? []).map((r) => r.camp_id))); | |
| setCamps((data as Camp[]) || []); | |
| } | |
| } catch { | |
| setLoadError('Camps could not be loaded.'); | |
| } finally { | |
| setLoading(false); | |
| } | |
| })(); | |
| }} |
🧰 Tools
🪛 React Doctor (0.5.8)
[warning] 398-398: This can cause a hydration mismatch because new Date() reached from JSX gives a different value on the server than in the browser. Move it into useEffect+useState to run only in the browser, or add suppressHydrationWarning to the parent if it's on purpose.
Move time or random values into useEffect+useState so they only run in the browser, or add suppressHydrationWarning to the parent if it's intentional
(rendering-hydration-mismatch-time)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/`(dashboard)/dashboard/camps/page.tsx around lines 380 -
409, The retry flow in the camps page is only refreshing the camp list and is
skipping the player-specific registration state rebuild. Update the `onRetry`
logic in `page.tsx` to mirror the initial player load path in the camps page
component by also fetching and restoring `registeredCamps` for the `isPlayer &&
player` branch. Make sure the same player data-loading helper or equivalent
logic used in the initial load is reused here so the rendered action buttons
stay consistent after a retry.
Comment on lines
+23
to
+31
| const ctx = await getActiveBaseballContext(); | ||
| if (!ctx?.activeTeamId) { | ||
| return ( | ||
| <div className="min-h-full bg-transparent flex items-center justify-center"> | ||
| <div className="text-center"> | ||
| <h1 className="text-xl font-semibold text-warm-900 mb-2">No Team Found</h1> | ||
| <p className="text-warm-600">You must be on a team to access documents.</p> | ||
| </div> | ||
| <div className="p-6 lg:p-8"> | ||
| <EmptyState | ||
| type="generic" | ||
| title="Join a team to see documents" | ||
| description="Once you join a baseball team, shared files and resources will show up here." | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
A failed active-context lookup now renders as a healthy “join a team” state.
src/app/baseball/(dashboard)/dashboard/documents/page.tsx:23-31 maps every falsy context to EmptyState, but src/lib/baseball/active-context.ts:128-173 returns null both for “user has no memberships” and for caught membership-resolution errors. That means transient auth/read-model failures are misreported as normal empty state, and the user loses any retry/error affordance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/`(dashboard)/dashboard/documents/page.tsx around lines 23 -
31, The documents page is treating every missing context as a normal “join a
team” empty state, even when getActiveBaseballContext() failed due to an actual
membership-resolution error. Update page.tsx to distinguish “no active
team/memberships” from a failed lookup returned by active-context.ts, and render
an error/retry state for the latter instead of EmptyState; use the
getActiveBaseballContext and EmptyState paths to locate the fallback logic.
Comment on lines
+36
to
+37
| title: `${access.displayName} - ${player.primary_position} | Helm`, | ||
| description: `View ${access.displayName}'s baseball recruiting profile. Class of ${player.grad_year}.`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the nullable primary_position in the page title.
access.displayName is non-null here, but player.primary_position is not — when unset, the public/SEO-indexed title renders Name - null | Helm. This is exactly the surface that gets shared and crawled for a recruiting profile.
🐛 Fix
- title: `${access.displayName} - ${player.primary_position} | Helm`,
- description: `View ${access.displayName}'s baseball recruiting profile. Class of ${player.grad_year}.`,
+ title: player.primary_position
+ ? `${access.displayName} - ${player.primary_position} | Helm`
+ : `${access.displayName} | Helm`,
+ description: `View ${access.displayName}'s baseball recruiting profile.${player.grad_year ? ` Class of ${player.grad_year}.` : ''}`,📝 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
| title: `${access.displayName} - ${player.primary_position} | Helm`, | |
| description: `View ${access.displayName}'s baseball recruiting profile. Class of ${player.grad_year}.`, | |
| title: player.primary_position | |
| ? `${access.displayName} - ${player.primary_position} | Helm` | |
| : `${access.displayName} | Helm`, | |
| description: `View ${access.displayName}'s baseball recruiting profile.${player.grad_year ? ` Class of ${player.grad_year}.` : ''}`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/`(public)/player/[id]/page.tsx around lines 36 - 37, The
page title in the player profile metadata is concatenating a nullable
player.primary_position, which can render “null” in shared/SEO-indexed titles.
Update the title generation in the page.tsx metadata logic to guard
player.primary_position and fall back to a safe label or omit the position when
it is unset, while keeping access.displayName as-is.
Comment on lines
+361
to
+369
| try { | ||
| return await updateEligibilityAction(id, data); | ||
| } catch (error) { | ||
| await logServerError( | ||
| `[Baseball Academics] Unexpected error: ${error instanceof Error ? error.message : String(error)}`, | ||
| { action: 'academics.updateEligibility', featureArea: 'baseball-academics' }, | ||
| ); | ||
| return mapAcademicsActionError<BaseballAcademicEligibility>(error); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Redundant logging re-emits control-flow errors to Sentry.
withBaseballAction already logs every failure: control-flow errors as warning with skipSentry: true, unexpected errors at full detail. This exported catch then calls logServerError(...) (default severity error) for all thrown errors, so BaseballUnauthorizedError/BaseballCapabilityError (normal auth denials) get re-emitted to Sentry + admin_events at error level — defeating the wrapper's skipSentry design. Same pattern repeats in createEligibilityRecord/upsertPlayerAcademics here and in the other refactored action files in this PR.
Drop the redundant logServerError in the exported catch and rely on the wrapper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/academics.ts` around lines 361 - 369, Remove the
redundant exported catch logging in Baseball Academics so control-flow failures
are not re-emitted to Sentry: in the try/catch around updateEligibilityAction,
let withBaseballAction handle logging and only keep the error-to-domain mapping
via mapAcademicsActionError. Apply the same fix to the related exported action
handlers in this refactor, such as createEligibilityRecord and
upsertPlayerAcademics, and remove any direct logServerError calls from these
outer catches.
Comment on lines
+395
to
+401
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const { data: updated, error } = await (supabase as any) | ||
| .from('baseball_academic_eligibility') | ||
| .update({ ...data, updated_by: ctx.user.id }) | ||
| .eq('id', id) | ||
| .select() | ||
| .single(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Mass assignment: updateEligibilityAction spreads raw client data into the update.
update({ ...data, updated_by: ctx.user.id }) passes unvalidated client input straight to Supabase. TS types are erased at runtime, so a crafted call can set any column on the row (team_id, player_id, etc.) and move/corrupt the record cross-team. The sibling createEligibilityRecordAction (Line 453) and upsertPlayerAcademicsAction (Line 528) both parse() first — this update path does not.
As per path instructions: "Validate every input with zod before passing to Supabase" and "Never trust client-supplied user/team/coach IDs."
🛡️ Whitelist fields instead of spreading raw input
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
- const { data: updated, error } = await (supabase as any)
- .from('baseball_academic_eligibility')
- .update({ ...data, updated_by: ctx.user.id })
+ const updateData: Record<string, unknown> = { updated_by: ctx.user.id };
+ if (data.gpa !== undefined) updateData.gpa = data.gpa;
+ if (data.credits_completed !== undefined) updateData.credits_completed = data.credits_completed;
+ if (data.credits_required !== undefined) updateData.credits_required = data.credits_required;
+ if (data.is_eligible !== undefined) updateData.is_eligible = data.is_eligible;
+ if (data.academic_standing !== undefined) updateData.academic_standing = data.academic_standing;
+ if (data.notes !== undefined) updateData.notes = data.notes;
+ // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
+ const { data: updated, error } = await (supabase as any)
+ .from('baseball_academic_eligibility')
+ .update(updateData)
.eq('id', id)
.select()
.single();📝 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
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| const { data: updated, error } = await (supabase as any) | |
| .from('baseball_academic_eligibility') | |
| .update({ ...data, updated_by: ctx.user.id }) | |
| .eq('id', id) | |
| .select() | |
| .single(); | |
| const updateData: Record<string, unknown> = { updated_by: ctx.user.id }; | |
| if (data.gpa !== undefined) updateData.gpa = data.gpa; | |
| if (data.credits_completed !== undefined) updateData.credits_completed = data.credits_completed; | |
| if (data.credits_required !== undefined) updateData.credits_required = data.credits_required; | |
| if (data.is_eligible !== undefined) updateData.is_eligible = data.is_eligible; | |
| if (data.academic_standing !== undefined) updateData.academic_standing = data.academic_standing; | |
| if (data.notes !== undefined) updateData.notes = data.notes; | |
| // eslint-disable-next-line `@typescript-eslint/no-explicit-any` | |
| const { data: updated, error } = await (supabase as any) | |
| .from('baseball_academic_eligibility') | |
| .update(updateData) | |
| .eq('id', id) | |
| .select() | |
| .single(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/academics.ts` around lines 395 - 401,
`updateEligibilityAction` is spreading unvalidated client `data` directly into
the Supabase update, which allows arbitrary column writes; fix this by
validating the input with the same zod-based pattern used in
`createEligibilityRecordAction` and `upsertPlayerAcademicsAction` before any DB
call. In `updateEligibilityAction`, replace the raw spread with a whitelist of
allowed updatable fields from the parsed payload and keep `updated_by` set from
`ctx.user.id` only. Do not accept or forward client-supplied IDs or ownership
fields such as `team_id` or `player_id`; only pass server-approved fields to
`.update()`.
Source: Path instructions
Comment on lines
+3
to
+14
| const REVOKED = new Set(['suspended', 'removed', 'invited']); | ||
|
|
||
| function filterStaffRows( | ||
| rows: Array<{ team_id: string; status: string | null }>, | ||
| ): string[] { | ||
| return rows | ||
| .filter((row) => { | ||
| const status = typeof row.status === 'string' ? row.status : null; | ||
| return !status || !REVOKED.has(status); | ||
| }) | ||
| .map((row) => row.team_id); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test reimplements the filter instead of exercising production code — provides no regression protection.
filterStaffRows/REVOKED here are a parallel copy of the logic in src/lib/baseball/active-context.ts:104-107, not an import of it. A future regression in loadMemberships (wrong status literal, dropped continue, reordered check) will not be caught by this suite because it only asserts against its own duplicate implementation.
Export the status-filter as a small pure function from active-context.ts (or from the shared capabilities.ts constant suggested in that file's review) and import it here:
// active-context.ts
export function isRevokedStaffStatus(status: string | null): boolean {
return status !== null && REVOKED_STATUSES.has(status);
}// this test
import { isRevokedStaffStatus } from '../active-context';
...
expect(isRevokedStaffStatus('invited')).toBe(true);
expect(isRevokedStaffStatus(null)).toBe(false);This also collapses the third copy of the suspended/removed/invited literal set referenced in the active-context.ts review comment.
🤖 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/__tests__/active-context-staff-status.test.ts` around lines
3 - 14, The test duplicates the staff-status filtering logic instead of
exercising the production implementation, so it won’t catch regressions in
loadMemberships or the revoked-status check. Move the status predicate out of
active-context.ts into a shared exported helper such as isRevokedStaffStatus (or
the shared constant/module referenced there), then import that helper into
active-context-staff-status.test.ts and assert its behavior directly. Keep the
revoked-status literals centralized so this test and active-context.ts use the
same source of truth.
Comment on lines
+22
to
+24
| vi.mock('@/lib/supabase/server', () => ({ | ||
| createClient: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the canonical Supabase test client instead of an inline mock.
This file mocks @/lib/supabase/server directly (createClient: vi.fn()) rather than going through the shared test client. As per path instructions, "Mock Supabase via the test client in src/test/." Even though createClient isn't directly exercised by the assertions here, keeping a second, ad-hoc Supabase mock pattern around invites drift (e.g. someone copies this pattern into a test that does need realistic query-builder chaining and it silently returns undefined).
🤖 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/__tests__/server-route-guards.test.ts` around lines 22 - 24,
The test is mocking `@/lib/supabase/server` inline with a bare createClient stub
instead of using the shared canonical Supabase test client. Update
server-route-guards.test.ts to import and use the test client helper from
src/test/ so the mock behavior stays consistent across tests, and remove the
ad-hoc vi.fn() mock for createClient. Keep the test wired through the shared
Supabase test utilities rather than a local replacement.
Source: Path instructions
| ? supabase | ||
| .from('baseball_team_coach_staff') | ||
| .select('team_id, created_at') | ||
| .select('team_id, created_at, status') |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Triplicated revoked-status literals — extract a shared constant.
['suspended', 'removed', 'invited'] is now hardcoded in three places: src/lib/baseball/capabilities.ts (REVOKED_STATUSES), here, and src/lib/baseball/__tests__/active-context-staff-status.test.ts. If the canonical status list ever changes (e.g. a new pending_reapproval status is added to the lifecycle), only this inline check is at risk of silently going stale — which means a staff member who should be denied could still resolve into activeTeamId/activeRole here even though capabilities.ts correctly denies them, producing an inconsistent (and exploitable) access-control surface.
Export REVOKED_STATUSES from capabilities.ts and import it here and in the test:
🔒 Suggested fix
+ import { REVOKED_STATUSES } from '`@/lib/baseball/capabilities`';
...
for (const row of staffMemberships.data ?? []) {
const status = typeof row.status === 'string' ? row.status : null;
- if (status === 'suspended' || status === 'removed' || status === 'invited') {
+ if (status !== null && REVOKED_STATUSES.has(status)) {
continue;
}Also applies to: 104-107
🤖 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/active-context.ts` at line 86, The revoked-status list is
duplicated in active-context logic and can drift from the canonical source in
capabilities.ts. Export REVOKED_STATUSES from src/lib/baseball/capabilities.ts,
then import and use it in active-context.ts wherever the inline
suspended/removed/invited check appears so the activeTeamId/activeRole
resolution stays aligned. Also update
src/lib/baseball/__tests__/active-context-staff-status.test.ts to reference the
same shared constant instead of hardcoding the status array, using the existing
active-context and REVOKED_STATUSES symbols to locate the affected checks.
Comment on lines
+128
to
+135
| // 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; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the supabase as any escape hatch.
src/lib/baseball/read-models/roster.ts:129 bypasses query typing for baseball_player_aggregates. Use the existing untyped helper pattern instead of suppressing no-explicit-any.
Proposed fix
import { createClient } from '`@/lib/supabase/server`';
+import { fromUntyped } from '`@/lib/supabase/untyped`';
import type { BaseballPlayerAggregates } from '`@/lib/types`';
@@
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
- const { data: aggregatesData, error: aggregatesError } = await (supabase as any)
- .from('baseball_player_aggregates')
+ const { data: aggregatesData, error: aggregatesError } = await fromUntyped(
+ supabase,
+ 'baseball_player_aggregates',
+ )
.select('*')
.eq('team_id', teamId) as {As per coding guidelines, “Avoid using any type annotations; use explicit TypeScript types from @/lib/types.”
📝 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
| // 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; | |
| }; | |
| import { createClient } from '`@/lib/supabase/server`'; | |
| import { fromUntyped } from '`@/lib/supabase/untyped`'; | |
| import type { BaseballPlayerAggregates } from '`@/lib/types`'; | |
| const { data: aggregatesData, error: aggregatesError } = await fromUntyped( | |
| supabase, | |
| 'baseball_player_aggregates', | |
| ) | |
| .select('*') | |
| .eq('team_id', teamId) as { | |
| data: BaseballPlayerAggregates[] | null; | |
| error: unknown; | |
| }; |
🤖 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 128 - 135, The roster
query is bypassing type safety with a `supabase as any` cast in the
`baseball_player_aggregates` fetch. Replace that escape hatch in `roster.ts`
with the existing untyped helper pattern used elsewhere for Supabase reads, and
keep the result typed via explicit `BaseballPlayerAggregates` types rather than
suppressing `no-explicit-any`.
Source: Coding guidelines
Comment on lines
+33
to
+39
| export function resolveReadModelLoadState(input: ReadModelLoadInput): ReadModelLoadState { | ||
| if (!input.authorized) return 'unauthorized'; | ||
| if (input.error) return input.partial ? 'partial' : 'error'; | ||
| if (!input.hasData) return 'empty'; | ||
| if (input.partial) return 'partial'; | ||
| return 'ready'; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
partial flag is shadowed by the !hasData check — contradicts the documented contract.
Per the docstring (lines 26-32), partial: true should resolve to 'partial' regardless of hasData. But the current order means {authorized: true, error: null, hasData: false, partial: true} returns 'empty', not 'partial' — silently downgrading a partial-failure-with-zero-rows into the "healthy empty state" copy (readModelStateLabel('empty')), which is the exact failure mode this module's banner comment says it exists to prevent.
🐛 Suggested fix — check `partial` before `!hasData`
export function resolveReadModelLoadState(input: ReadModelLoadInput): ReadModelLoadState {
if (!input.authorized) return 'unauthorized';
if (input.error) return input.partial ? 'partial' : 'error';
- if (!input.hasData) return 'empty';
if (input.partial) return 'partial';
+ if (!input.hasData) return 'empty';
return 'ready';
}Add a regression test for {authorized: true, error: null, hasData: false, partial: true} → 'partial'.
📝 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
| export function resolveReadModelLoadState(input: ReadModelLoadInput): ReadModelLoadState { | |
| if (!input.authorized) return 'unauthorized'; | |
| if (input.error) return input.partial ? 'partial' : 'error'; | |
| if (!input.hasData) return 'empty'; | |
| if (input.partial) return 'partial'; | |
| return 'ready'; | |
| } | |
| export function resolveReadModelLoadState(input: ReadModelLoadInput): ReadModelLoadState { | |
| if (!input.authorized) return 'unauthorized'; | |
| if (input.error) return input.partial ? 'partial' : 'error'; | |
| if (input.partial) return 'partial'; | |
| if (!input.hasData) return 'empty'; | |
| return 'ready'; | |
| } |
🤖 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/product-trust/read-model-state.ts` around lines 33 - 39, The
resolveReadModelLoadState function is checking !input.hasData before
input.partial, which causes partial:true with no data to incorrectly return
empty instead of partial. Update the evaluation order in
resolveReadModelLoadState so the partial flag is handled before the !hasData
branch, preserving the documented contract for ReadModelLoadState. Add a
regression test covering the authorized, no-error, hasData:false, partial:true
case to ensure it resolves to partial.
Owner
Author
There was a problem hiding this comment.
Actionable comments posted: 70
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/baseball/nav-registry.ts (1)
683-692: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't hide the shared Events surface for non-showcase programs.
Line 692 now restricts
/baseball/dashboard/eventstoshowcase|academy|club, but Lines 687-690 describe this as a shared team-events page for both coaches and players. Unlikeorganization/teams, the supplied server guard only applies showcase program gating to org routes insrc/lib/baseball/server-route-guards.ts:41-84, so college, high-school, and JUCO teams lose the nav entry without a matching route-level restriction.🤖 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/nav-registry.ts` around lines 683 - 692, The shared Events nav entry is being incorrectly hidden by the program-type restriction on the nav config. Update the Events item in nav-registry so it remains visible to non-showcase team programs, and only rely on the existing server-side guard behavior for org routes in server-route-guards; keep the shared team-events surface aligned with the comments and leave the route-level access control to the appropriate guard logic.src/lib/baseball/active-context.ts (1)
75-86: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply status filtering to player memberships too.
Line 79 still does not select player membership
status, so removed/inactive/pending player rows can become the active team context while staff rows are now correctly filtered. Selectstatusonbaseball_team_membersand skip non-current player rows before pushing the membership.Proposed fix
- .select('team_id, created_at') + .select('team_id, created_at, status') ... for (const row of playerMemberships.data ?? []) { + const status = typeof row.status === 'string' ? row.status : null; + if (status === 'pending' || status === 'inactive' || status === 'removed') { + continue; + } memberships.push({Also applies to: 103-107
🤖 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/active-context.ts` around lines 75 - 86, The player membership branch in active-context logic still omits status filtering, so inactive or pending player rows can be chosen as the active team context. Update the membership query in active-context.ts for baseball_team_members to select status like the staff path does, then apply the same current-status check used for staff before adding player memberships to the candidate list. Make sure the filtering is handled in the membership assembly logic around the active-context selection so both player and staff paths behave consistently.Source: Path instructions
src/app/baseball/actions/recruiting-philosophy.ts (1)
362-399: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate
playerIdsbefore calculating match scores.Line 396 calls
calculate_player_match_scorefor every client-supplied player ID using the coach ID, but this function does not prove those players are recruitable/visible to the coach. Resolve the active team and filter through the public-profile/recruitability access resolver before invoking the RPC. As per path instructions, “Never trust client-supplied user/team/coach IDs.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/baseball/actions/recruiting-philosophy.ts` around lines 362 - 399, Gate client-supplied playerIds inside calculateMatchScoresForPlayers before calling calculate_player_match_score: resolve the active team/coach context, verify each player is recruitable or visible to that coach through the same public-profile/access resolver used elsewhere, and only pass authorized IDs into the RPC. Keep the existing authentication and coach lookup in place, but add the authorization filter in the loop or immediately before it so unauthorized player IDs never reach the scoring query.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 @.coderabbit/semgrep/helmv3.yml:
- Around line 250-254: The extra pattern in
helmv3-security-definer-without-search-path is dead because the regexp after
alter\s+function cannot match real function signatures, so it never contributes
beyond the broader ALTER FUNCTION check. Remove the redundant pattern-not-regex
or rewrite it in the helmv3.yml rule so it correctly covers the intended ALTER
FUNCTION ... SET search_path cases, using the existing pattern-not-regex entries
around security definer and alter function as the reference point.
In `@e2e/baseball-box-score.spec.ts`:
- Line 126: Tighten the stayedOnForm check in the baseball box score E2E test so
it only matches the actual create-form route and no longer treats every
/dashboard URL as failure-state. Update the predicate in the test that uses
currentUrl to distinguish the create form from the success redirect emitted by
NewGameClient, which navigates to /baseball/dashboard/stats/games/${id}. Ensure
the assertion uses a more specific URL match for the form path so success and
failure are no longer conflated.
In `@scripts/__tests__/scripts-no-committed-secrets.test.mjs`:
- Line 9: The HARDCODED_JWT matcher in the secrets test is too narrow because it
only matches one HS256 header and can miss other valid JWTs such as ES256.
Update the pattern used in the test to the broader JWT-shape regex already used
by the helmv3-hardcoded-supabase-credentials-py semgrep rule, so the
scripts/no-committed-secrets check catches any three-part JWT-like token
consistently.
- Around line 13-27: The secret-scan test is missing
scripts/seed-rini-baseball-demo.ts because BASEBALL_SEED_GLOB in listSeedScripts
only matches names starting with seed-baseball-. Update the seed filename filter
so it also includes the rini baseball demo seed script, and keep the traversal
logic in listSeedScripts unchanged so that all intended seed files are collected
and scanned.
In `@scripts/route-crawler-baseball.mjs`:
- Around line 49-83: The baseball crawler auth flow is treating real login
failures the same as missing credentials, letting `main()` exit 0 and skip the
crawl. In `signIn()` and `main()` from `scripts/route-crawler-baseball.mjs`,
remove the unused GET to `/baseball/login`, use the actual login entry point at
`/baseball/login`, and make authentication errors return a non-null failure
signal so the script exits 1 when login/bootstrap fails instead of silently
passing CI.
In `@scripts/seed-baseball-roster.mjs`:
- Around line 3-16: Load environment values from .env.local before reading the
seed variables in seed-baseball-roster.mjs so the script follows the
repo-standard Node script behavior. Update the startup flow around SUPABASE_URL,
SERVICE_KEY, ORG_ID, and COACH_ID to initialize dotenv first, then fall back to
process.env for missing values. Keep the existing validation checks and error
messages, but ensure the script can run locally without manually exporting all
four variables.
In `@scripts/seed-baseball-stats.mjs`:
- Around line 141-146: The session seed flow in scripts/seed-baseball-stats.mjs
is still non-idempotent because the row key built in the session creation logic
uses a randomized sessionDate, causing a new id set on every rerun and leaving
stale rows behind. Update the id generation in the session-building section so
it is derived only from stable identifiers such as teamId, playerId, and coachId
(and any other deterministic inputs), and make the upsert path that uses
onConflict: 'id' rely on that stable key so re-running the script produces the
same records instead of new ones.
- Around line 170-184: The seeder currently writes only legacy stats tables and
bypasses the canonical baseball stats pipeline. Update the logic in
seed-baseball-stats.mjs around the sessions/hitting/fielding/pitching flow so it
persists source-of-truth data into baseball_games, baseball_box_score_batting,
and baseball_box_score_pitching instead of only
baseball_player_stats/baseball_player_aggregates, then invoke
recalculate_baseball_season_stats after inserts. Keep the existing helper flow
(e.g. upsertRow and the session processing loop) but change it to seed canonical
records so Stats Center and season rollups are populated consistently.
In `@src/app/baseball/`(dashboard)/dashboard/calendar/page.tsx:
- Around line 61-65: The calendar lookup in the dashboard page is resolving
`teamId` from `baseball_team_members` using only `player_id` and
`maybeSingle()`, which breaks for players with multiple active memberships.
Update the `page.tsx` flow to resolve the team through the active Baseball
context helper/selection logic already used for role/team validation, and derive
the calendar team from that active context instead of assuming a single
membership row. Make sure the `teamId` selection in this path can handle
multi-team players consistently so the calendar loads for the currently active
team.
In `@src/app/baseball/`(dashboard)/dashboard/camps/page.tsx:
- Around line 259-262: The retry path in the camps page only reloads camps and
leaves player registration state stale, so make the initial load and retry both
use the same fetch routine from the camps page logic (the flow that reads
`baseball_camps` and `baseball_camp_registrations`). Update the catch/reset path
around the camp loading state to clear or refresh `registeredCamps`, and have
the retry handler call that shared loader instead of duplicating a partial
fetch. This keeps `registeredCamps` synchronized with `setCamps` after failures
and retries.
In `@src/app/baseball/`(dashboard)/dashboard/compare/CompareClient.tsx:
- Around line 155-163: The retry action in CompareClient is wired to
router.refresh(), but the compare data is fetched in the client-side
fetchPlayers flow and that refresh will not clear loadError or rerun the effect.
Refactor CompareClient so fetchPlayers is extracted as a reusable callback (for
example around the existing useEffect logic) and invoke that callback from
ReadModelStateNotice’s onRetry handler instead of router.refresh(). Keep the
retry path aligned with the same state updates used by the initial client fetch.
In `@src/app/baseball/`(dashboard)/dashboard/dev-plan/page.tsx:
- Around line 390-391: The `showToast` function from `useToast()` is recreated
on each render, so the `handleComplete` and `handleUncomplete` callbacks in the
dev-plan page need to list it in their `useCallback` dependency arrays. Update
both callbacks to include `showToast` alongside the existing dependencies so
they stay in sync with the latest toast function returned by `useToast`.
In `@src/app/baseball/`(dashboard)/dashboard/documents/page.tsx:
- Around line 23-34: The documents page is treating every null from
getActiveBaseballContext() as a normal no-team empty state, which hides real
context resolution failures. Update getActiveBaseballContext in
active-context.ts to return an explicit failure signal or rethrow when context
lookup/logged errors occur, and then adjust the documents page branch that
currently renders EmptyState so it can distinguish “no membership” from
operational errors and render ReadModelStateNotice for failures.
In `@src/app/baseball/`(dashboard)/dashboard/roster/RosterClient.tsx:
- Around line 204-208: The fallback query in RosterClient should stay fully
typed and avoid pulling every column from baseball_player_aggregates. Replace
the supabase as any escape hatch with an explicit Supabase/type-safe query using
the existing BaseballPlayerAggregates type from `@/lib/types`, and scope the
select in the aggregates fetch to only the fields this client component actually
renders. Keep the shape consistent in the aggregatesData branch so the fallback
remains narrow, typed, and easy to maintain.
- Around line 127-151: The RosterClient useEffect reads serverTeamId but does
not list it in the dependency array, so updates to that prop can leave
resolvedTeamId stale. Update the dependency list for this effect in RosterClient
to include serverTeamId alongside selectedTeamId, authLoading, and
coach?.organization_id, keeping the existing resolution order between
serverTeamId, selectedTeamId, and the org lookup intact.
In `@src/app/baseball/`(dashboard)/dashboard/stats/games/create/NewGameClient.tsx:
- Around line 156-173: The start-time control in NewGameClient is missing an
accessible label, so add a proper label or an aria-label for the conditional
type="time" input rendered when createCalendarEvent is true. Keep the control
discoverable by screen readers and ensure it matches the existing input
accessibility requirements in this component, alongside the current focus-ring
styling and keyboard-friendly behavior.
- Around line 16-18: The initial date in NewGameClient is using
Date.toISOString(), which can shift the displayed day because it is UTC-based.
Update the default for the gameDate state to use the user’s local calendar date
instead, keeping the logic in NewGameClient and the today/gameDate
initialization consistent with local time rather than UTC.
In `@src/app/baseball/`(dashboard)/dashboard/stats/games/create/page.tsx:
- Around line 11-31: In the create game page, preflight the same capability that
the games action enforces by checking can_manage_stats on the loaded coach
before rendering NewGameClient, alongside the existing coach_type and
organization_id checks. Also handle the supabase .single() results in the page
component (for both the baseball_coaches and baseball_teams queries) by
detecting query errors separately from missing records, and throw on load
failure instead of redirecting to command center/program so real data-fetch
issues surface correctly.
In `@src/app/baseball/`(public)/player/[id]/page.tsx:
- Around line 35-38: The metadata generated in the page-level function is
interpolating nullable player fields directly, which can produce public titles
and descriptions containing null. Update the metadata construction in the page
component’s metadata function to guard `player.primary_position` and
`player.grad_year` before building the strings, using fallbacks or conditional
text so the title and description never render null values.
- Around line 41-49: The shared public-profile access check is being executed
twice because generateMetadata() and PublicPlayerProfilePage both call
createClient(), auth.getUser(), and resolvePublicProfileAccess(). Move that
lookup into a cached helper using React 19 cache() (or an equivalent shared
cached function) and have both generateMetadata and PublicPlayerProfilePage call
the same helper so the access gate is resolved once per request.
In `@src/app/baseball/actions/__tests__/change-password.test.ts`:
- Around line 17-29: The change-password action test is missing a mock for the
server error logger, so the wrong-password path in changePasswordAction can
exercise real Sentry/admin-event plumbing instead of staying isolated. Add a
vi.mock for "`@/lib/server-error-logger`" alongside the existing mocks in
change-password.test.ts, and ensure the mocked logServerError is used when the
wrong-password branch runs so the test remains hermetic.
In `@src/app/baseball/actions/academics.ts`:
- Around line 424-434: The `error instanceof z.ZodError` branch in
`createEligibilityRecordAction` is unreachable because `withBaseballAction`
converts the thrown validation error into a `BaseballActionError` before this
`catch`, so invalid input falls back to the generic error. Move the Zod
validation handling into the action body where `eligibilitySchema.parse(...)`
runs, and apply the same fix to `upsertPlayerAcademics` around its `parse` call
so field-level validation messages are returned instead of the generic message.
- Around line 395-401: `updateEligibilityAction` is persisting `data` directly
into Supabase without the same zod validation used by `createEligibilityRecord`
and `upsertPlayerAcademics`. Add a schema validation step before the
`baseball_academic_eligibility.update(...)` call so `gpa`, `credits`,
`academic_standing`, and `notes` are validated/sanitized first, and only the
parsed result is spread into the update payload. Use the existing
`updateEligibilityAction` flow and the Supabase update block as the place to
enforce this before `updated_by` is added.
In `@src/app/baseball/actions/calendar.ts`:
- Around line 221-227: The partial update path in the calendar action is
overwriting existing timestamps: `buildDateTime` and `buildEndDateTime` are
being called with missing fields, which can reset `start_time` or set `end_time`
to null. Update the event update flow in `calendar.ts` to first load the current
event timestamps, then merge incoming `input.startDate`, `input.startTime`,
`input.endDate`, and `input.endTime` with the existing values before assigning
`updateData.start_time` and `updateData.end_time`. Focus on the logic around the
calendar update handler that builds `updateData` so partial edits preserve the
prior date/time components.
- Around line 153-161: The attendance insert in the calendar action currently
trusts client-supplied attendeeIds, so validate every ID against resolvedTeamId
before building attendanceRecords. In the action that inserts into
baseball_event_attendance, fetch or verify that each playerId belongs to the
resolved team and reject the request if any ID is outside that team. Keep the
existing insert logic in calendar.ts, but add the guard before the map/insert
path and ensure the input is validated with zod before anything reaches
Supabase.
- Around line 153-177: The RSVP and game creation branches in calendar action
ignore insert failures, so the flow can return success even when
`baseball_event_attendance` or `baseball_games` rows were not created. Update
the `calendar.ts` action around the `input.requiresRsvp` and `input.eventType`
checks to capture and handle insert errors from the
`supabase.from(...).insert(...)` calls, and propagate/fail the operation instead
of continuing silently. If possible, keep the behavior consistent for the event
creation path by ensuring the linked records are only considered successful when
the inserts in these branches succeed.
In `@src/app/baseball/actions/camps.ts`:
- Around line 74-83: The catch block in withBaseballAction is re-logging every
thrown error through logServerError, which turns handled typed failures like
BaseballCapabilityError into Sentry-worthy noise. Update the wrapper around
deleteCampAction to only call logServerError for genuinely unexpected errors,
and let the known action errors continue through mapCampsActionError without an
extra error-severity log. Apply the same filtering pattern in the analogous
wrappers identified by camps.ts, dev-plans.ts, travel.ts, and watchlist.ts.
In `@src/app/baseball/actions/dev-plans.ts`:
- Around line 224-230: The player goal completion actions are incorrectly
protected by the coach settings capability, which blocks the player-only
dashboard flow. Update completeGoalAction and uncompleteGoalAction in
dev-plans.ts to remove requiredCapability: 'can_manage_settings' and instead
authorize using ctx.activePlayerId together with the target plan’s player_id, or
split the coach and player paths into separate actions. Keep the existing
withBaseballAction wiring but make the permission check align with the
dashboard’s player-owned usage.
In `@src/app/baseball/actions/documents.ts`:
- Around line 570-575: The version history assembly in getVersionHistory is
failing the entire list because Promise.all over versionsData aborts when
signDocumentStoragePath throws for one bad storage_path. Update the
versionsWithUrls mapping to tolerate per-version signing failures, either by
using Promise.allSettled or by catching errors inside the per-version transform
and falling back to a null/empty file_url, so the rest of the versions still
render even if one URL cannot be signed.
In `@src/app/baseball/actions/games.ts`:
- Around line 204-213: The `createGame` wrapper in `games.ts` is duplicating
error logging because `withBaseballAction` already logs thrown errors via
`logServerException`. Remove the extra `logServerError` call from this `catch`
block and keep only `mapGameActionError(error)` for the returned user-facing
error. Apply the same pattern to the other public action wrappers mentioned in
the review (`academics.ts`, `announcements.ts`, and `tasks.ts`) so the wrapper
remains the single logging site.
- Around line 507-543: `saveBoxScoreBattingAction` is doing a non-transactional
DELETE-then-INSERT, which can wipe existing batting rows if the insert fails.
Update the write path in `saveBoxScoreBattingAction` to use the same atomic RPC
pattern as `saveFullBoxScore` (`save_baseball_full_box_score`) or switch to an
`upsert`-based approach so rows are not lost on partial failure. Also apply the
same fix to the matching pitching path (`saveBoxScorePitchingAction`) since it
uses the same pattern.
- Around line 692-700: The recalculation loop in markGameCompleted is swallowing
rpc failures because db2.rpc returns a result object instead of throwing. Update
the Promise.all flow around recalculate_baseball_season_stats so each call
checks the returned error and surfaces it, then fail the completion path if any
player recalculation fails. Use the existing allPlayerIds mapping and the rpc
call to preserve the current behavior for successful runs while preventing
silent stale baseball_player_season_stats data.
In `@src/app/baseball/actions/interests.ts`:
- Around line 70-140: The business-rule failures in addToInterestsAction are
being thrown as BaseballActionError, but withBaseballAction does not treat that
as expected control flow, so the messages are lost and the wrapper logs them as
unhandled. Update the addToInterestsAction body to return the existing result
shape with error text for cases like missing player, missing organization,
invalid organization type, and insert failure, and keep throws only for truly
unexpected issues. Make sure the caller path through mapInterestActionError can
preserve the returned error message, and use the existing symbols
withBaseballAction, mapInterestActionError, and addToInterestsAction to keep the
fix localized.
In `@src/app/baseball/actions/lineups.ts`:
- Around line 160-177: The `baseball_lineup_positions` save flow in `lineups.ts`
is doing a non-atomic delete followed by insert, which can leave a lineup empty
if the insert fails. Update this path to be atomic by using the project’s
existing safer pattern: either an `upsert` with targeted pruning or a
transactional RPC such as `save_baseball_lineup_positions`, and make sure the
fix is applied in the same save action that currently deletes and reinserts
positions. Keep the existing `supabase`-based flow, but remove the
DELETE-then-INSERT sequence so partial failures cannot cause silent data loss.
In `@src/app/baseball/actions/onboarding.ts`:
- Around line 461-472: The baseball signup flow in onboarding.ts is currently
enabling recruiting for every non-college player, which conflicts with the
privacy-first onboarding behavior used elsewhere. Update the recruitingActivated
logic in the signup path so it defaults to off and only turns on when the user
explicitly opts in, and make sure the insert into baseball_players and the
CommonSchemas.recruitingPlayerState.parse call both use that same opt-in value.
Refer to the onboarding flow around the recruitingActivated variable and the
baseball_players insert to keep the signup behavior consistent.
- Around line 220-229: Drop the client-trusted user shortcut from
completeCoachOnboarding so the exported server action only accepts data and
always derives the authenticated user internally; keep the pre-verified fast
path confined to runCompleteCoachOnboardingCore and remove the _preVerifiedUser
parameter from the public entrypoint. Also update the onboarding defaults in the
same module so recruiting_activated is not auto-enabled for non-college players
during signup; leave it false until the explicit activation flow handled by the
recruiting activation action.
In `@src/app/baseball/actions/philosophy.ts`:
- Around line 71-113: The savePhilosophySettingsAction currently writes
client-provided numeric fields from PhilosophySettings straight into
baseball_coach_philosophy without schema checks. Add Zod validation in
savePhilosophySettingsAction before building philosophyData to verify
alertSensitivity, declineThreshold, pressureGapThreshold, bubbleZoneRange, and
the priority_* weights are finite numbers within the expected ranges and reject
invalid/NaN/negative values. Keep the existing auth/coaching checks, then only
proceed to the Supabase upsert after the validated values are passed through.
- Around line 57-68: The outer catch in savePhilosophySettings is re-logging all
failures as unexpected errors, including the handled auth/capability cases
already logged by withBaseballAction. Update savePhilosophySettings to
distinguish BaseballActionError or other expected action failures from truly
unexpected exceptions, and only call logServerError for the unexpected path; for
handled failures, just return mapPhilosophyActionError(error) without a second
error log. Use the existing savePhilosophySettingsAction, withBaseballAction,
and mapPhilosophyActionError flow to keep logging consistent.
In `@src/app/baseball/actions/recruiting-philosophy.ts`:
- Around line 132-162: Handle the `.single()` lookup in
`updateRecruitingPhilosophy` by checking the read error explicitly: only treat
`PGRST116` as “not found” and return a sanitized DB error for any other failure.
Update the existing write/delete paths in `recruiting-philosophy.ts` to stop
returning raw `*.message` values and instead use `sanitizeDbError(...)` for
every Supabase error response, including the related delete logic around the
other referenced block.
- Around line 108-119: The wrapper around saveRecruitingPhilosophyAction is
double-logging errors by calling logServerError for every caught exception
before mapRecruitingPhilosophyActionError runs, which makes handled
auth/capability failures noisy. Update this catch path so it only logs truly
unexpected errors or lets withBaseballAction/BaseballActionError handling remain
the source of expected warnings, and keep the mapping logic in place for the
returned failure shape. Apply the same fix to the other affected wrapper
functions in this diff, using the existing action names and map*ActionError
helpers to locate them.
In `@src/app/baseball/actions/stat-event-imports.ts`:
- Around line 408-437: The commit/review decision in stat-event-imports.ts still
trusts args.detectionAutoCommit whenever args.rawFileBody is missing, which lets
callers bypass the GAP-4 hold; update the logic around validateEventRows,
serverAutoCommit, and requiresReview so the server does not fall back to the
client value. Either make rawFileBody mandatory for this path or fail closed by
treating the import as hold_for_review (or rejecting it) when detectFromParse
cannot run, while keeping the existing do_not_commit guard intact.
In `@src/app/baseball/actions/teams.ts`:
- Around line 575-582: The invite action call path in TeamSettingsClient needs
error handling because generateTeamInviteCode/regenerateTeamInviteCode can
reject with BaseballUnauthorizedError, BaseballNoActiveTeamError, or
BaseballCapabilityError before returning TeamInviteResult. Wrap the await in the
TeamSettingsClient invite handler with try/catch, and on failure route the error
into the existing error state/toast flow instead of letting it bypass the UI
feedback.
In `@src/app/baseball/actions/travel.ts`:
- Around line 157-166: The `z.ZodError` branch in `createItineraryAction` is
unreachable because `withBaseballAction` wraps thrown validation errors before
they reach this `catch`, so the field-level message is lost. Move validation in
`createItineraryAction`, `updateItinerary`, and `addExpense` to
`safeParse`/`safeParseAsync` before any throw, and return the specific schema
error message directly as a `success: false` result so it bypasses
`BaseballActionError` wrapping. Keep the existing
`logServerError`/`mapTravelActionError` path only for unexpected non-validation
failures.
In `@src/app/baseball/actions/watchlist.ts`:
- Around line 48-56: The z.ZodError handling in mapWatchlistActionError is dead
because withBaseballAction re-wraps parse failures before the public wrapper
sees them. Update the watchlist actions that call WatchlistSchemas.*.parse(...)
to use safeParse instead and return a success:false result with the schema issue
message directly, so invalid input is surfaced from the action and the
z.ZodError branch in mapWatchlistActionError can be removed.
In `@src/components/baseball/ReadModelStateNotice.tsx`:
- Around line 37-49: The ReadModelStateNotice component is currently rendered as
static content, so screen readers may not announce async failure or unauthorized
state changes. Update the wrapper in ReadModelStateNotice to include an
accessible notification mechanism such as role="alert" or an aria-live region,
and keep the announcement on the top-level Card/container so users are alerted
when the state changes.
In `@src/contracts/baseball/coachhelm-product-truth.contract.test.ts`:
- Around line 1-6: The shared repo/read helper is duplicated across the contract
tests, so extract the repeated process.cwd/readFileSync/join logic into a new
test-utils module and import it from coachhelm-product-truth, demo-stats-smoke,
product-trust, and route-shell. Update the contract test files to use the
exported read helper from test-utils instead of redefining repo and read
locally, keeping the existing describe/expect/it usage unchanged.
- Around line 9-31: The contract tests in
coachhelm-product-truth.contract.test.ts are too weak because they only search
for loose source tokens and won’t fail on the regressions named by the test
titles. Tighten the assertions in the tests that read composites.ts,
outcome-sweep.ts, and stats-center.ts so they verify the actual contract shape
or co-occurrence of the expected symbols instead of a single generic substring;
use the concrete exports/functions in those modules (for example, the outcome
sweep registry entry and the staff-gated stats envelope) and make the checks
specific enough that bare claims, raw-delta-only logic, or missing
authorized:false envelope handling will fail.
In `@src/contracts/baseball/product-trust.contract.test.ts`:
- Around line 17-27: These contract tests are too loose because the bare regex
alternations in DailyBriefPanel and StatsCenterClient can pass on unrelated
occurrences of insights, error, or empty. Tighten the assertions in
product-trust.contract.test.ts by updating the checks around read(...) for
DailyBriefPanel and StatsCenterClient to match only the specific
empty-state/user-facing copy, removing the generic alternatives so the tests
verify the intended distinction between load errors and empty states.
In `@src/hooks/use-baseball-auth.ts`:
- Around line 134-148: The `run()` flow in `use-baseball-auth` can leave the
auth guard stuck loading if `verifyServerSession()` rejects before reaching the
final state updates. Wrap the `await verifyServerSession()` path in `run()` with
a `try/catch`, and on any failure fail closed by clearing auth, setting
`authorized` false, and always calling `setLoading(false)` in the success and
error paths (or a `finally`). Keep the existing `cancelled`, `reconcileStore`,
`setRole`, and `setAuthorized` behavior unchanged for the successful case.
- Around line 97-128: reconcileStore in use-baseball-auth.ts is repopulating the
auth store from a stale snapshot after store.clear(), which can merge old
coach/player fields back into the new account state. Update the logic so the
post-clear setCoach and setPlayer calls build from the fresh
coachProfile/playerProfile data only, without spreading store.coach or
store.player captured before clearing. Also make sure the verified current
user/profile state is written back into the auth store after reconciliation so
the cache reflects the latest authenticated account.
In `@src/lib/baseball/__tests__/active-context-staff-status.test.ts`:
- Around line 17-24: The regression test in the active-context staff status
suite is missing a `removed` fixture even though the scenario is meant to cover
it. Update the `filterStaffRows` test in `active-context-staff-status.test.ts`
to include a row with `status: 'removed'` alongside the existing invited and
suspended cases, and keep the expectation asserting that `removed` is excluded,
so the behavior in `roster.ts` stays covered if exclusion logic changes.
In `@src/lib/baseball/__tests__/roster-read-model.test.ts`:
- Around line 4-26: The current test only checks `RosterReadModel` object
literals, so replace it with behavior-based coverage for `getRoster()`. Mock
`createClient()` and drive `getRoster()` through a roster query failure to
assert `rosterError: true`, and through a successful empty roster to assert
`rosterError: false` with empty members. Keep the assertions focused on the
`getRoster` path and its returned shape rather than type-only literals.
In `@src/lib/baseball/nav-registry.ts`:
- Around line 189-205: Enforce the mutual exclusion between
requiredAnyCapabilities and requiredCapability in the BaseBall nav registry type
and resolution logic, instead of relying on the comment. Update the entry shape
in nav-registry.ts so the compiler rejects configs that set both fields
together, and adjust the visibility handling in the registry builder/matcher
around the requiredAnyCapabilities path so it does not silently override
requiredCapability in the logic that processes entries. Use the existing
nav-entry type and the code near the capability check to make the XOR explicit
at compile time and runtime.
In `@src/lib/baseball/public-profile-access.ts`:
- Around line 116-140: The public-profile access logic in the membership lookup
path is using an arbitrary first row from baseball_team_members, which makes
dual-roster players evaluate nondeterministically. Update the logic in the
public profile access function to consider all relevant memberships
deterministically, using an explicit rule or aggregation before applying
baseball_program_settings and getDefaultProgramSettings, rather than relying on
whichever team_id Supabase returns first.
- Around line 81-90: Restrict the staff bypass in public-profile access to
active memberships only. Update the membership lookup in public-profile access
so the `memberships` loop only considers active `baseball_team_members` records
before calling `coachHasTeamAccess`, preventing former-team coaches from
reaching the `staff` fast-path; use the existing `playerId`, `viewerCoachId`,
and `coachHasTeamAccess` flow to locate and tighten the query.
In `@src/lib/baseball/read-models/command-center-adapter.ts`:
- Around line 53-57: The load-state mapping in command-center-adapter should
preserve usable data when a sub-read fails. In the code that calls
resolveReadModelLoadState(), pass a partial flag based on model.error and
hasData so partial-loaded roster/events/risk data does not get masked as a full
error. Update the loadState calculation near
model.authorized/model.error/hasData to include partial: Boolean(model.error &&
hasData).
In `@src/lib/baseball/read-models/command-center.ts`:
- Around line 238-249: The weekEvents range is currently anchored to the
server’s current date instead of the requested forDate, which can make it
disagree with todayEvents. Update the week boundary calculation in
command-center.ts so the weekStart and weekEnd logic inside the
includeWeekEvents block derives from forDate when it is provided, falling back
to now only when forDate is absent. Keep the existing weekStartIso and
weekEndIso flow, but ensure the date source is consistent with the rest of the
read model.
- Around line 257-262: The roster stats lookup in command-center is selecting
the first aggregate row from baseball_player_aggregates, which can return the
wrong team for multi-team players. Update the logic in the relevant
command-center flow to use the teamId-specific aggregate by matching on team_id
=== teamId (or filtering the embedded relation by team_id) before assigning
stats, so the selected row always corresponds to the current team.
In `@src/lib/baseball/read-models/roster.ts`:
- Around line 128-135: The `roster.ts` query is bypassing Supabase type safety
by casting the client to `any`; update the `baseball_player_aggregates` read in
the roster read model to use the typed `supabase` client directly. Keep the
`from(...).select(...).eq(...)` chain on the existing client, and if the
generated row type needs narrowing, cast only the returned `data` to
`BaseballPlayerAggregates[]` rather than the client itself. Use the
`aggregatesData`/`aggregatesError` destructuring in this block as the place to
remove the `any` escape hatch.
In `@src/lib/baseball/recruitability.ts`:
- Around line 39-44: Update the roster/discoverability helpers in
recruitability.ts so they only consider active memberships from
baseball_team_members. In the helpers that build the team member set and
evaluate discoverability/on_own_roster, add the status constraint to the
Supabase query so transferred or released rows are excluded. Use the existing
helper functions around the members lookup (including the roster/discoverability
logic referenced by the current select/in chain) and keep the filtering
consistent in both places.
In `@src/lib/baseball/server-route-guards.ts`:
- Around line 101-102: Use the resolved session role from auth instead of
checking raw coach-profile presence in the route guard. In
`requirePlayerRoute`/the redirect logic in `server-route-guards.ts`, base the
coach redirect only on `session.role === 'coach'` so a dual-profile session with
`role: 'player'` is allowed into player routes. Then add a regression test in
`server-route-guards.test.ts` for a `{ role: 'player', coach: {...}, player:
{...} }` session to verify it does not redirect to the coach dashboard and still
reaches player routes.
In `@supabase/migrations/20260630000000_baseball_save_full_box_score_rpc.sql`:
- Around line 56-57: The full box score save path currently uses destructive
DELETEs before re-inserting rows, which violates the atomic upsert requirement.
In the save function that handles `p_game_id` for baseball box scores, change
the flow to stage and validate incoming batting/pitching rows first, then
persist them with `INSERT ... ON CONFLICT DO UPDATE` using the existing
canonical row keys, and only prune stale rows after validation and successful
writes. Keep the logic localized to the `baseball_box_score_batting` and
`baseball_box_score_pitching` persistence steps so the save remains atomic
before any rollup work.
- Around line 62-90: The batting insert in the baseball_save_full_box_score RPC
is trusting client-supplied player IDs inside the SECURITY DEFINER function, so
add a membership check before inserting each row. In the batting loop where
v_bat is processed, verify that the payload player_id belongs to the same team
as v_team_id and skip or reject the insert if it does not; apply the same guard
to the pitching loop for v_pit as well. Use the existing loop variables and
insert logic in this migration so the fix is localized to the RPC’s batting and
pitching sections.
- Around line 14-17: Harden the `baseball_save_full_box_score` function
definition by replacing the permissive `SECURITY DEFINER` search path with an
explicit empty `SET search_path = ''`, since the function already
schema-qualifies its application objects. Add an inline comment immediately
above the `SECURITY DEFINER` declaration explaining why elevated privileges are
required, so the intent is documented and the function matches the security
guidance.
In `@supabase/migrations/20260630180000_baseball_scope_player_ids_rls.sql`:
- Around line 39-43: The SECURITY DEFINER helper in this migration is using an
unsafe search path and is missing the required elevation justification. Update
the function definition so the search path is explicitly empty via SET
search_path = '' and schema-qualify any referenced objects inside the helper.
Also add the inline comment documenting why the SECURITY DEFINER elevation is
required, matching the existing RLS-related helper pattern.
- Around line 48-50: The helper in public.get_my_baseball_player_id scope
checking is allowing self-access too early; update the logic so the
authenticated player is only granted access after confirming they belong to the
requested p_team_id. Adjust the relevant RLS helper/policy flow in the migration
so the p_player_id == public.get_my_baseball_player_id() path is gated by the
team membership check rather than returning true immediately.
- Around line 3-29: The backfill in the baseball_team_coach_staff migration
casts legacy JSON values to UUIDs inside the subquery, so a single malformed
historical value can abort the whole update. Update the transformation logic in
the UPDATE ... FROM subquery to guard the casts before ARRAY(...) is built,
using the existing JSON shape checks around player_scope and the player_ids
branches. Apply the same UUID validation/filtering to both the object-shaped
player_scope->'player_ids' path and the array-shaped player_scope path, or fail
explicitly with a clear diagnostic before casting.
In `@supabase/migrations/20260630180200_baseball_team_invitation_redeem_rpc.sql`:
- Around line 3-41: The two SECURITY DEFINER RPCs expose mutable invitation
counters to any authenticated caller, letting them increment or decrement
`used_count` for arbitrary invitation UUIDs. Refactor
`try_redeem_baseball_team_invitation` and
`release_baseball_team_invitation_redemption` so the membership insert and
counter change happen atomically in one authorization-checked path, or replace
the counter-based release flow with a caller-scoped reservation/ledger that can
only be released by the same user. Keep the execute grants off any function that
can mutate `baseball_team_invitations.used_count` without verifying ownership or
a valid reservation.
In `@vercel.json`:
- Around line 6-11: The Vercel configuration currently lives in vercel.json, but
the project convention prefers vercel.ts. When this config is next touched,
migrate the existing deploymentEnabled settings into the vercel.ts configuration
entry point and remove the JSON-based config so the Vercel setup follows the
repository standard.
- Around line 7-10: The deploymentEnabled rule is disabling preview deployments
for all non-main branches, which prevents the Lighthouse preview flow from
getting Vercel preview URLs. Update the deploymentEnabled configuration in
vercel.json to avoid the blanket wildcard disable and instead use explicit
branch exclusions or a more targeted allowlist, keeping main enabled while
allowing preview branches needed for commit-based checks.
---
Outside diff comments:
In `@src/app/baseball/actions/recruiting-philosophy.ts`:
- Around line 362-399: Gate client-supplied playerIds inside
calculateMatchScoresForPlayers before calling calculate_player_match_score:
resolve the active team/coach context, verify each player is recruitable or
visible to that coach through the same public-profile/access resolver used
elsewhere, and only pass authorized IDs into the RPC. Keep the existing
authentication and coach lookup in place, but add the authorization filter in
the loop or immediately before it so unauthorized player IDs never reach the
scoring query.
In `@src/lib/baseball/active-context.ts`:
- Around line 75-86: The player membership branch in active-context logic still
omits status filtering, so inactive or pending player rows can be chosen as the
active team context. Update the membership query in active-context.ts for
baseball_team_members to select status like the staff path does, then apply the
same current-status check used for staff before adding player memberships to the
candidate list. Make sure the filtering is handled in the membership assembly
logic around the active-context selection so both player and staff paths behave
consistently.
In `@src/lib/baseball/nav-registry.ts`:
- Around line 683-692: The shared Events nav entry is being incorrectly hidden
by the program-type restriction on the nav config. Update the Events item in
nav-registry so it remains visible to non-showcase team programs, and only rely
on the existing server-side guard behavior for org routes in
server-route-guards; keep the shared team-events surface aligned with the
comments and leave the route-level access control to the appropriate guard
logic.
🪄 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: 5d91d952-ecf6-4663-9950-835c5a39f4a9
⛔ Files ignored due to path filters (2)
.ultracode/baseballhelm/cc-4878.logis excluded by!**/*.log.ultracode/baseballhelm/cc.logis excluded by!**/*.log
📒 Files selected for processing (147)
.circleci/README.md.circleci/config.yml.circleci/scripts/wait-for-vercel-preview.sh.coderabbit/semgrep/helmv3.yml.gitignore.ultracode/baseballhelm/events.ndjsondocs/BASEBALLHELM_BUG_AUDIT_2026-06-30.mddocs/operations/BASEBALL_STATS_SOURCE_OF_TRUTH.mde2e/baseball-box-score.spec.tse2e/baseball-phase1.spec.tsscripts/__tests__/baseball-action-integrity.test.mjsscripts/__tests__/baseball-demo-seed-surfaces.test.mjsscripts/__tests__/baseball-stale-route-links.test.mjsscripts/__tests__/route-crawler-baseball.test.mjsscripts/__tests__/scripts-no-committed-secrets.test.mjsscripts/__tests__/seed-baseball-stats.safety.test.mjsscripts/__tests__/seed-recruiting-invariant.test.mjsscripts/baseballhelm-bug-audit/create-issues.mjsscripts/baseballhelm-bug-audit/issues.mjsscripts/route-crawler-baseball.mjsscripts/seed-baseball-demo.tsscripts/seed-baseball-roster.mjsscripts/seed-baseball-stats.mjsscripts/seed-rini-baseball-demo.tssrc/app/baseball/(dashboard)/_components/hub-definitions.tssrc/app/baseball/(dashboard)/_components/hub-sub-nav.tsxsrc/app/baseball/(dashboard)/_components/resolve-active-hub.tssrc/app/baseball/(dashboard)/dashboard/announcements/page.tsxsrc/app/baseball/(dashboard)/dashboard/calendar/page.tsxsrc/app/baseball/(dashboard)/dashboard/camps/page.tsxsrc/app/baseball/(dashboard)/dashboard/command-center/page.tsxsrc/app/baseball/(dashboard)/dashboard/compare/CompareClient.tsxsrc/app/baseball/(dashboard)/dashboard/dev-plan/page.tsxsrc/app/baseball/(dashboard)/dashboard/discover/DiscoverClient.tsxsrc/app/baseball/(dashboard)/dashboard/documents/page.tsxsrc/app/baseball/(dashboard)/dashboard/import/page.tsxsrc/app/baseball/(dashboard)/dashboard/my-stats/page.tsxsrc/app/baseball/(dashboard)/dashboard/roster/RosterClient.tsxsrc/app/baseball/(dashboard)/dashboard/roster/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/NewGameClient.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/error.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/loading.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/new/page.tsxsrc/app/baseball/(dashboard)/dashboard/travel/page.tsxsrc/app/baseball/(public)/packet/[token]/csv/route.tssrc/app/baseball/(public)/player/[id]/page.tsxsrc/app/baseball/actions/__tests__/change-password.test.tssrc/app/baseball/actions/__tests__/interests.test.tssrc/app/baseball/actions/__tests__/save-full-box-score.test.tssrc/app/baseball/actions/__tests__/team-join-code.test.tssrc/app/baseball/actions/academics.tssrc/app/baseball/actions/announcements.tssrc/app/baseball/actions/auth.tssrc/app/baseball/actions/calendar.tssrc/app/baseball/actions/camps.tssrc/app/baseball/actions/dev-plans.tssrc/app/baseball/actions/discover.tssrc/app/baseball/actions/documents.tssrc/app/baseball/actions/games.tssrc/app/baseball/actions/imports.tssrc/app/baseball/actions/interests.tssrc/app/baseball/actions/lineups.tssrc/app/baseball/actions/onboarding.tssrc/app/baseball/actions/philosophy.tssrc/app/baseball/actions/player-peek.tssrc/app/baseball/actions/recruiting-philosophy.tssrc/app/baseball/actions/stat-event-imports.tssrc/app/baseball/actions/tasks.tssrc/app/baseball/actions/teams.tssrc/app/baseball/actions/travel.tssrc/app/baseball/actions/watchlist.tssrc/app/golf/actions/demo-access.tssrc/app/golf/actions/insight-celebration.tssrc/app/golf/actions/message-attachments.tssrc/app/golf/actions/push-notifications.tssrc/app/golf/actions/round-recap.tssrc/app/golf/actions/v3/focus-area-progress.tssrc/app/golf/actions/v3/goal-progress.tssrc/components/CommandPalette.tsxsrc/components/baseball/ReadModelStateNotice.tsxsrc/components/baseball/command-center/CommandCenterClient.tsxsrc/components/baseball/dashboard-shell.tsxsrc/components/baseball/games/GamesList.tsxsrc/components/features/college-card.tsxsrc/contracts/baseball/coachhelm-product-truth.contract.test.tssrc/contracts/baseball/demo-stats-smoke.contract.test.tssrc/contracts/baseball/product-trust.contract.test.tssrc/contracts/baseball/route-shell.contract.test.tssrc/hooks/use-baseball-auth.tssrc/hooks/use-dashboard.tssrc/lib/auth/supabase-rate-limit.tssrc/lib/baseball/__tests__/active-context-staff-status.test.tssrc/lib/baseball/__tests__/command-palette-nav.test.tssrc/lib/baseball/__tests__/documents-capability.test.tssrc/lib/baseball/__tests__/import-source-enabled.test.tssrc/lib/baseball/__tests__/import-source-registration.test.tssrc/lib/baseball/__tests__/nav-performance-capability.test.tssrc/lib/baseball/__tests__/resolve-active-hub.test.tssrc/lib/baseball/__tests__/resolve-team.test.tssrc/lib/baseball/__tests__/roster-read-model.test.tssrc/lib/baseball/__tests__/server-route-guards.test.tssrc/lib/baseball/__tests__/showcase-org-nav.test.tssrc/lib/baseball/__tests__/stats-route-aliases.test.tssrc/lib/baseball/active-context.tssrc/lib/baseball/capabilities.tssrc/lib/baseball/capability-groups.tssrc/lib/baseball/command-palette-nav.tssrc/lib/baseball/import-source-enabled.tssrc/lib/baseball/nav-registry.tssrc/lib/baseball/public-profile-access.tssrc/lib/baseball/read-models/__tests__/command-center.test.tssrc/lib/baseball/read-models/command-center-adapter.tssrc/lib/baseball/read-models/command-center.tssrc/lib/baseball/read-models/roster.tssrc/lib/baseball/recruitability.tssrc/lib/baseball/resolve-team-server.tssrc/lib/baseball/resolve-team.tssrc/lib/baseball/server-route-guards.tssrc/lib/baseball/stats-route-aliases.tssrc/lib/baseball/with-baseball-action.tssrc/lib/db-error.tssrc/lib/product-trust/__tests__/read-model-state.test.tssrc/lib/product-trust/read-model-state.tssrc/lib/types/baseball-staff-roles.tssrc/lib/validation/action-schemas.tssrc/lib/validation/server-action-validator.tssupabase/investigations/shot_type_findings.mdsupabase/investigations/shot_type_investigation.sqlsupabase/migrations/20260624000050_baseball_rls_helpers_and_policies.sqlsupabase/migrations/20260624000061_baseball_lifting_performance.sqlsupabase/migrations/20260624000063_baseball_v11_premium_lifting.sqlsupabase/migrations/20260624000080_baseball_elite_stat_event_model.sqlsupabase/migrations/20260624000090_baseball_settings_os.sqlsupabase/migrations/20260624000092_baseball_signals_and_actions.sqlsupabase/migrations/20260624000093_baseball_postgame_reviews.sqlsupabase/migrations/20260624000094_baseball_practice_effectiveness.sqlsupabase/migrations/20260624000221_baseball_video_links_and_class_conflicts.sqlsupabase/migrations/20260624000230_baseball_signal_action_materialization.sqlsupabase/migrations/20260624000450_baseball_ai_audit_log.sqlsupabase/migrations/20260630000000_baseball_save_full_box_score_rpc.sqlsupabase/migrations/20260630170000_baseball_college_recruiting_check.sqlsupabase/migrations/20260630180000_baseball_scope_player_ids_rls.sqlsupabase/migrations/20260630180100_baseball_documents_capability.sqlsupabase/migrations/20260630180200_baseball_team_invitation_redeem_rpc.sqlvercel.json
| const currentUrl = page.url(); | ||
| const wentToGamePage = currentUrl.includes('/stats/games/'); | ||
| const stayedOnForm = currentUrl.includes('/stats/games/new') || currentUrl.includes('/dashboard'); | ||
| const stayedOnForm = currentUrl.includes('/stats/games/create') || currentUrl.includes('/dashboard'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tighten the “stayed on form” predicate.
e2e/baseball-box-score.spec.ts:126 treats any /dashboard URL as “still on the form”, but the success path from src/app/baseball/(dashboard)/dashboard/stats/games/create/NewGameClient.tsx:48-49 is /baseball/dashboard/stats/games/${id}, which also matches /dashboard. This makes the assertion unable to tell success from failure.
Suggested fix
- const stayedOnForm = currentUrl.includes('/stats/games/create') || currentUrl.includes('/dashboard');
+ const stayedOnForm =
+ /\/baseball\/dashboard\/stats\/games\/create(?:[/?#]|$)/.test(currentUrl);📝 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 stayedOnForm = currentUrl.includes('/stats/games/create') || currentUrl.includes('/dashboard'); | |
| const stayedOnForm = | |
| /\/baseball\/dashboard\/stats\/games\/create(?:[/?#]|$)/.test(currentUrl); |
🤖 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 `@e2e/baseball-box-score.spec.ts` at line 126, Tighten the stayedOnForm check
in the baseball box score E2E test so it only matches the actual create-form
route and no longer treats every /dashboard URL as failure-state. Update the
predicate in the test that uses currentUrl to distinguish the create form from
the success redirect emitted by NewGameClient, which navigates to
/baseball/dashboard/stats/games/${id}. Ensure the assertion uses a more specific
URL match for the form path so success and failure are no longer conflated.
Comment on lines
+3
to
+16
| const SUPABASE_URL = process.env.NEXT_PUBLIC_SUPABASE_URL ?? process.env.SUPABASE_URL; | ||
| const SERVICE_KEY = process.env.SUPABASE_SERVICE_ROLE_KEY; | ||
| const ORG_ID = process.env.BASEBALL_SEED_ORG_ID; | ||
| const COACH_ID = process.env.BASEBALL_SEED_COACH_ID; | ||
|
|
||
| if (!SUPABASE_URL || !SERVICE_KEY) { | ||
| console.error('Set NEXT_PUBLIC_SUPABASE_URL (or SUPABASE_URL) and SUPABASE_SERVICE_ROLE_KEY.'); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (!ORG_ID || !COACH_ID) { | ||
| console.error('Set BASEBALL_SEED_ORG_ID and BASEBALL_SEED_COACH_ID for roster seed targets.'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Load .env.local before consuming seed env vars.
scripts/seed-baseball-roster.mjs:3-16 now hard-fails unless callers export four variables manually. That breaks the repo-standard local script path, where .env.local is the first source of truth. As per path instructions, "Node scripts must read env from .env.local first, then process.env."
🤖 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 `@scripts/seed-baseball-roster.mjs` around lines 3 - 16, Load environment
values from .env.local before reading the seed variables in
seed-baseball-roster.mjs so the script follows the repo-standard Node script
behavior. Update the startup flow around SUPABASE_URL, SERVICE_KEY, ORG_ID, and
COACH_ID to initialize dotenv first, then fall back to process.env for missing
values. Keep the existing validation checks and error messages, but ensure the
script can run locally without manually exporting all four variables.
Source: Path instructions
Comment on lines
+141
to
+146
| const rowId = detId(`${teamId}:${playerId}:${sessionDate}:${s2}`); | ||
| sessions.push({ | ||
| id: rowId, | ||
| player_id: playerId, | ||
| team_id: TEAM_ID, | ||
| coach_id: COACH_ID, | ||
| team_id: teamId, | ||
| coach_id: coachId, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The session upsert is still non-idempotent.
scripts/seed-baseball-stats.mjs:121-146 folds a freshly randomized sessionDate into the row id, so the upsert(... onConflict: 'id') at scripts/seed-baseball-stats.mjs:170-182 generates a different keyset on each rerun. Old baseball_player_stats rows remain, while the aggregate write later only reflects the latest in-memory sample. As per path instructions, "One-off ops scripts. Must be idempotent (re-running is safe)."
Also applies to: 170-184
🤖 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 `@scripts/seed-baseball-stats.mjs` around lines 141 - 146, The session seed
flow in scripts/seed-baseball-stats.mjs is still non-idempotent because the row
key built in the session creation logic uses a randomized sessionDate, causing a
new id set on every rerun and leaving stale rows behind. Update the id
generation in the session-building section so it is derived only from stable
identifiers such as teamId, playerId, and coachId (and any other deterministic
inputs), and make the upsert path that uses onConflict: 'id' rely on that stable
key so re-running the script produces the same records instead of new ones.
Source: Path instructions
Comment on lines
+170
to
+184
| for (const session of sessions) { | ||
| const { error: statsErr } = await upsertRow( | ||
| client, | ||
| 'baseball_player_stats', | ||
| session, | ||
| dryRun, | ||
| `${playerId} ${session.session_date}`, | ||
| ); | ||
| if (statsErr) { | ||
| console.error(` ✗ ${playerId}:`, statsErr.message); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // Recompute aggregates | ||
| const hitting = sessions.filter(ss => ss.at_bats > 0); | ||
| const hitting = sessions.filter((ss) => ss.at_bats > 0); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
This seeder still bypasses the canonical stats pipeline.
scripts/seed-baseball-stats.mjs:170-257 only populates baseball_player_stats and baseball_player_aggregates. BaseballHelm’s source-of-truth path is baseball_games + baseball_box_score_batting/baseball_box_score_pitching, followed by recalculate_baseball_season_stats, so this script can report success while Stats Center and season rollups stay empty or stale. Based on learnings, "baseball_games, baseball_box_score_batting, and baseball_box_score_pitching are the canonical persisted stats sources of truth" and "seeds must not create an empty Stats Center with populated legacy-only rows."
Also applies to: 225-257
🤖 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 `@scripts/seed-baseball-stats.mjs` around lines 170 - 184, The seeder currently
writes only legacy stats tables and bypasses the canonical baseball stats
pipeline. Update the logic in seed-baseball-stats.mjs around the
sessions/hitting/fielding/pitching flow so it persists source-of-truth data into
baseball_games, baseball_box_score_batting, and baseball_box_score_pitching
instead of only baseball_player_stats/baseball_player_aggregates, then invoke
recalculate_baseball_season_stats after inserts. Keep the existing helper flow
(e.g. upsertRow and the session processing loop) but change it to seed canonical
records so Stats Center and season rollups are populated consistently.
Source: Learnings
Comment on lines
+61
to
+65
| const { data: teamMember } = await supabase | ||
| .from('baseball_team_members') | ||
| .select('team_id') | ||
| .eq('player_id', playerId) | ||
| .maybeSingle(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve player calendars through the active Baseball context.
Line 61 queries baseball_team_members by player_id only and expects a single row. Baseball players can have multiple active team memberships, and the active-context helper already validates the selected team against membership rows. With multiple rows, this path can fail or leave teamId null, producing an empty calendar for multi-team players.
Proposed fix
+import { getActiveBaseballContext } from '`@/lib/baseball/active-context`';
...
} else if (!isCoach) {
- const playerId = session.player?.id;
- if (playerId) {
- const { data: teamMember } = await supabase
- .from('baseball_team_members')
- .select('team_id')
- .eq('player_id', playerId)
- .maybeSingle();
- teamId = teamMember?.team_id || null;
- }
+ const activeContext = await getActiveBaseballContext();
+ teamId = activeContext?.activeTeamId ?? null;
}As per path instructions, BaseballHelm role/team flows must be checked against CLAUDE.md, which allows multi-team player contexts.
📝 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: teamMember } = await supabase | |
| .from('baseball_team_members') | |
| .select('team_id') | |
| .eq('player_id', playerId) | |
| .maybeSingle(); | |
| import { getActiveBaseballContext } from '`@/lib/baseball/active-context`'; | |
| ... | |
| } else if (!isCoach) { | |
| const activeContext = await getActiveBaseballContext(); | |
| teamId = activeContext?.activeTeamId ?? null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/`(dashboard)/dashboard/calendar/page.tsx around lines 61 -
65, The calendar lookup in the dashboard page is resolving `teamId` from
`baseball_team_members` using only `player_id` and `maybeSingle()`, which breaks
for players with multiple active memberships. Update the `page.tsx` flow to
resolve the team through the active Baseball context helper/selection logic
already used for role/team validation, and derive the calendar team from that
active context instead of assuming a single membership row. Make sure the
`teamId` selection in this path can handle multi-team players consistently so
the calendar loads for the currently active team.
Source: Path instructions
Comment on lines
+62
to
+90
| INSERT INTO public.baseball_box_score_batting ( | ||
| game_id, player_id, team_id, batting_order, | ||
| ab, r, h, doubles, triples, hr, rbi, bb, k, sb, cs, hbp, sac, sf, lob, | ||
| avg, obp, slg, ops | ||
| ) VALUES ( | ||
| p_game_id, | ||
| (v_bat->>'player_id')::uuid, | ||
| v_team_id, | ||
| NULLIF(v_bat->>'batting_order', '')::integer, | ||
| COALESCE((v_bat->>'ab')::integer, 0), | ||
| COALESCE((v_bat->>'r')::integer, 0), | ||
| COALESCE((v_bat->>'h')::integer, 0), | ||
| COALESCE((v_bat->>'doubles')::integer, 0), | ||
| COALESCE((v_bat->>'triples')::integer, 0), | ||
| COALESCE((v_bat->>'hr')::integer, 0), | ||
| COALESCE((v_bat->>'rbi')::integer, 0), | ||
| COALESCE((v_bat->>'bb')::integer, 0), | ||
| COALESCE((v_bat->>'k')::integer, 0), | ||
| COALESCE((v_bat->>'sb')::integer, 0), | ||
| COALESCE((v_bat->>'cs')::integer, 0), | ||
| COALESCE((v_bat->>'hbp')::integer, 0), | ||
| COALESCE((v_bat->>'sac')::integer, 0), | ||
| COALESCE((v_bat->>'sf')::integer, 0), | ||
| COALESCE((v_bat->>'lob')::integer, 0), | ||
| NULLIF(v_bat->>'avg', '')::numeric, | ||
| NULLIF(v_bat->>'obp', '')::numeric, | ||
| NULLIF(v_bat->>'slg', '')::numeric, | ||
| NULLIF(v_bat->>'ops', '')::numeric | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate every payload player_id belongs to the game’s team before inserting.
Because this RPC is SECURITY DEFINER, RLS will not prevent a coach from submitting another team’s player_id and writing box-score rows stamped with v_team_id. Check membership for each batting and pitching line before insert.
Guard pattern
+ IF NOT EXISTS (
+ SELECT 1
+ FROM public.baseball_team_members btm
+ WHERE btm.team_id = v_team_id
+ AND btm.player_id = (v_bat->>'player_id')::uuid
+ ) THEN
+ RAISE EXCEPTION 'player % is not on team %', v_bat->>'player_id', v_team_id;
+ END IF;
+
INSERT INTO public.baseball_box_score_batting (Apply the same guard in the pitching loop before inserting v_pit.
As per path instructions, server code must not trust client-supplied user/team/player IDs.
Also applies to: 97-119
🤖 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/20260630000000_baseball_save_full_box_score_rpc.sql`
around lines 62 - 90, The batting insert in the baseball_save_full_box_score RPC
is trusting client-supplied player IDs inside the SECURITY DEFINER function, so
add a membership check before inserting each row. In the batting loop where
v_bat is processed, verify that the payload player_id belongs to the same team
as v_team_id and skip or reject the insert if it does not; apply the same guard
to the pitching loop for v_pit as well. Use the existing loop variables and
insert logic in this migration so the fix is localized to the RPC’s batting and
pitching sections.
Source: Path instructions
Comment on lines
+3
to
+29
| UPDATE public.baseball_team_coach_staff | ||
| SET scope_player_ids = sub.ids | ||
| FROM ( | ||
| SELECT | ||
| tcs.id, | ||
| CASE | ||
| WHEN jsonb_typeof(tcs.player_scope) = 'object' | ||
| AND tcs.player_scope ? 'player_ids' | ||
| AND jsonb_typeof(tcs.player_scope->'player_ids') = 'array' | ||
| AND jsonb_array_length(tcs.player_scope->'player_ids') > 0 | ||
| THEN ARRAY( | ||
| SELECT jsonb_array_elements_text(tcs.player_scope->'player_ids')::uuid | ||
| ) | ||
| WHEN jsonb_typeof(tcs.player_scope) = 'array' | ||
| AND jsonb_array_length(tcs.player_scope) > 0 | ||
| THEN ARRAY( | ||
| SELECT jsonb_array_elements_text(tcs.player_scope)::uuid | ||
| ) | ||
| ELSE NULL::uuid[] | ||
| END AS ids | ||
| FROM public.baseball_team_coach_staff tcs | ||
| WHERE tcs.scope_player_ids IS NULL | ||
| AND tcs.player_scope IS NOT NULL | ||
| ) sub | ||
| WHERE baseball_team_coach_staff.id = sub.id | ||
| AND sub.ids IS NOT NULL | ||
| AND cardinality(sub.ids) > 0; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the legacy UUID casts before backfilling.
Lines 14 and 19 cast legacy JSON text directly to uuid; one malformed historical value aborts the whole migration. Filter or explicitly fail with a diagnostic before casting.
Proposed safer cast pattern
- SELECT jsonb_array_elements_text(tcs.player_scope->'player_ids')::uuid
+ SELECT value::uuid
+ FROM jsonb_array_elements_text(tcs.player_scope->'player_ids') AS value
+ WHERE value ~* '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$'Apply the same filter to the array-shaped legacy branch.
📝 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
| UPDATE public.baseball_team_coach_staff | |
| SET scope_player_ids = sub.ids | |
| FROM ( | |
| SELECT | |
| tcs.id, | |
| CASE | |
| WHEN jsonb_typeof(tcs.player_scope) = 'object' | |
| AND tcs.player_scope ? 'player_ids' | |
| AND jsonb_typeof(tcs.player_scope->'player_ids') = 'array' | |
| AND jsonb_array_length(tcs.player_scope->'player_ids') > 0 | |
| THEN ARRAY( | |
| SELECT jsonb_array_elements_text(tcs.player_scope->'player_ids')::uuid | |
| ) | |
| WHEN jsonb_typeof(tcs.player_scope) = 'array' | |
| AND jsonb_array_length(tcs.player_scope) > 0 | |
| THEN ARRAY( | |
| SELECT jsonb_array_elements_text(tcs.player_scope)::uuid | |
| ) | |
| ELSE NULL::uuid[] | |
| END AS ids | |
| FROM public.baseball_team_coach_staff tcs | |
| WHERE tcs.scope_player_ids IS NULL | |
| AND tcs.player_scope IS NOT NULL | |
| ) sub | |
| WHERE baseball_team_coach_staff.id = sub.id | |
| AND sub.ids IS NOT NULL | |
| AND cardinality(sub.ids) > 0; | |
| UPDATE public.baseball_team_coach_staff | |
| SET scope_player_ids = sub.ids | |
| FROM ( | |
| SELECT | |
| tcs.id, | |
| CASE | |
| WHEN jsonb_typeof(tcs.player_scope) = 'object' | |
| AND tcs.player_scope ? 'player_ids' | |
| AND jsonb_typeof(tcs.player_scope->'player_ids') = 'array' | |
| AND jsonb_array_length(tcs.player_scope->'player_ids') > 0 | |
| THEN ARRAY( | |
| SELECT value::uuid | |
| FROM jsonb_array_elements_text(tcs.player_scope->'player_ids') AS value | |
| WHERE value ~* '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' | |
| ) | |
| WHEN jsonb_typeof(tcs.player_scope) = 'array' | |
| AND jsonb_array_length(tcs.player_scope) > 0 | |
| THEN ARRAY( | |
| SELECT value::uuid | |
| FROM jsonb_array_elements_text(tcs.player_scope) AS value | |
| WHERE value ~* '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' | |
| ) | |
| ELSE NULL::uuid[] | |
| END AS ids | |
| FROM public.baseball_team_coach_staff tcs | |
| WHERE tcs.scope_player_ids IS NULL | |
| AND tcs.player_scope IS NOT NULL | |
| ) sub | |
| WHERE baseball_team_coach_staff.id = sub.id | |
| AND sub.ids IS NOT NULL | |
| AND cardinality(sub.ids) > 0; |
🤖 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/20260630180000_baseball_scope_player_ids_rls.sql` around
lines 3 - 29, The backfill in the baseball_team_coach_staff migration casts
legacy JSON values to UUIDs inside the subquery, so a single malformed
historical value can abort the whole update. Update the transformation logic in
the UPDATE ... FROM subquery to guard the casts before ARRAY(...) is built,
using the existing JSON shape checks around player_scope and the player_ids
branches. Apply the same UUID validation/filtering to both the object-shaped
player_scope->'player_ids' path and the array-shaped player_scope path, or fail
explicitly with a clear diagnostic before casting.
Comment on lines
+39
to
+43
| LANGUAGE plpgsql | ||
| STABLE | ||
| SECURITY DEFINER | ||
| SET search_path = public, pg_temp | ||
| AS $$ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden the SECURITY DEFINER search path and document the elevation.
Line 42 uses public, pg_temp and the function lacks the required elevation comment. This helper is used by RLS, so keep the search path empty and schema-qualify all references.
Proposed fix
STABLE
SECURITY DEFINER
-SET search_path = public, pg_temp
+-- SECURITY DEFINER is required for RLS policies to evaluate staff/player
+-- visibility through stable helper functions without recursive policy checks.
+SET search_path = ''
AS $$As per path instructions, “Forbid SECURITY DEFINER functions without an explicit SET search_path = '' and an inline comment justifying the elevation.”
📝 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
| LANGUAGE plpgsql | |
| STABLE | |
| SECURITY DEFINER | |
| SET search_path = public, pg_temp | |
| AS $$ | |
| LANGUAGE plpgsql | |
| STABLE | |
| SECURITY DEFINER | |
| -- SECURITY DEFINER is required for RLS policies to evaluate staff/player | |
| -- visibility through stable helper functions without recursive policy checks. | |
| SET search_path = '' | |
| AS $$ |
🤖 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/20260630180000_baseball_scope_player_ids_rls.sql` around
lines 39 - 43, The SECURITY DEFINER helper in this migration is using an unsafe
search path and is missing the required elevation justification. Update the
function definition so the search path is explicitly empty via SET search_path =
'' and schema-qualify any referenced objects inside the helper. Also add the
inline comment documenting why the SECURITY DEFINER elevation is required,
matching the existing RLS-related helper pattern.
Source: Path instructions
Comment on lines
+48
to
+50
| IF p_player_id = public.get_my_baseball_player_id() THEN | ||
| RETURN true; | ||
| END IF; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope player self-access to the requested team.
Line 48 returns true for the authenticated player before checking whether that player currently belongs to p_team_id. Any RLS policy using this helper with historical or cross-team rows can expose that player’s team-scoped records outside the active team context.
Proposed fix
- IF p_player_id = public.get_my_baseball_player_id() THEN
- RETURN true;
+ IF p_player_id = public.get_my_baseball_player_id() THEN
+ RETURN EXISTS (
+ SELECT 1
+ FROM public.baseball_team_members btm
+ WHERE btm.team_id = p_team_id
+ AND btm.player_id = p_player_id
+ );
END IF;📝 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
| IF p_player_id = public.get_my_baseball_player_id() THEN | |
| RETURN true; | |
| END IF; | |
| IF p_player_id = public.get_my_baseball_player_id() THEN | |
| RETURN EXISTS ( | |
| SELECT 1 | |
| FROM public.baseball_team_members btm | |
| WHERE btm.team_id = p_team_id | |
| AND btm.player_id = p_player_id | |
| ); | |
| END IF; |
🤖 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/20260630180000_baseball_scope_player_ids_rls.sql` around
lines 48 - 50, The helper in public.get_my_baseball_player_id scope checking is
allowing self-access too early; update the logic so the authenticated player is
only granted access after confirming they belong to the requested p_team_id.
Adjust the relevant RLS helper/policy flow in the migration so the p_player_id
== public.get_my_baseball_player_id() path is gated by the team membership check
rather than returning true immediately.
Comment on lines
+3
to
+41
| CREATE OR REPLACE FUNCTION public.try_redeem_baseball_team_invitation(p_invitation_id uuid) | ||
| RETURNS boolean | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = public, pg_temp | ||
| AS $$ | ||
| DECLARE | ||
| v_id uuid; | ||
| BEGIN | ||
| UPDATE public.baseball_team_invitations | ||
| SET used_count = COALESCE(used_count, 0) + 1 | ||
| WHERE id = p_invitation_id | ||
| AND is_active = true | ||
| AND (expires_at IS NULL OR expires_at > now()) | ||
| AND (max_uses IS NULL OR COALESCE(used_count, 0) < max_uses) | ||
| RETURNING id INTO v_id; | ||
|
|
||
| RETURN v_id IS NOT NULL; | ||
| END; | ||
| $$; | ||
|
|
||
| REVOKE ALL ON FUNCTION public.try_redeem_baseball_team_invitation(uuid) FROM PUBLIC; | ||
| GRANT EXECUTE ON FUNCTION public.try_redeem_baseball_team_invitation(uuid) TO authenticated, service_role; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.release_baseball_team_invitation_redemption(p_invitation_id uuid) | ||
| RETURNS void | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path = public, pg_temp | ||
| AS $$ | ||
| BEGIN | ||
| UPDATE public.baseball_team_invitations | ||
| SET used_count = GREATEST(COALESCE(used_count, 0) - 1, 0) | ||
| WHERE id = p_invitation_id; | ||
| END; | ||
| $$; | ||
|
|
||
| REVOKE ALL ON FUNCTION public.release_baseball_team_invitation_redemption(uuid) FROM PUBLIC; | ||
| GRANT EXECUTE ON FUNCTION public.release_baseball_team_invitation_redemption(uuid) TO authenticated, service_role; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not expose mutable invitation counters as authenticated SECURITY DEFINER RPCs.
Line 25 and Line 41 grant arbitrary authenticated callers the ability to increment or decrement used_count for any known invitation UUID while bypassing RLS. In particular, release_baseball_team_invitation_redemption() can be called repeatedly to reduce used_count to 0, bypassing max_uses; try_redeem...() can also burn invite capacity. Move the membership insert and counter update into one authorization-checked RPC, or introduce a per-user/player reservation token/ledger and only allow releasing the caller’s own reservation. Based on learnings, box-score/team write paths must be atomic before downstream rollups or side effects run. <retrieved_learnings>
🤖 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/20260630180200_baseball_team_invitation_redeem_rpc.sql`
around lines 3 - 41, The two SECURITY DEFINER RPCs expose mutable invitation
counters to any authenticated caller, letting them increment or decrement
`used_count` for arbitrary invitation UUIDs. Refactor
`try_redeem_baseball_team_invitation` and
`release_baseball_team_invitation_redemption` so the membership insert and
counter change happen atomically in one authorization-checked path, or replace
the counter-based release flow with a caller-scoped reservation/ledger that can
only be released by the same user. Keep the execute grants off any function that
can mutate `baseball_team_invitations.used_count` without verifying ownership or
a valid reservation.
Source: Learnings
Comment on lines
250
to
+254
| patterns: | ||
| - pattern: SECURITY DEFINER | ||
| - pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,500}set\\s+search_path" | ||
| - pattern-regex: "(?i)security\\s+definer" | ||
| - pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,8000}set\\s+search_path" | ||
| - pattern-not-regex: "(?is)alter\\s+function[\\s\\S]{0,2000}set\\s+search_path" | ||
| - pattern-not-regex: "(?is)alter\\s+function[\\s'\\s%]*set\\s+search_path" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Dead regex in helmv3-security-definer-without-search-path — line 254 never matches real SQL.
[\s'\s%]* (the character class on line 254) only contains whitespace, ', and %. It can't span a function's name/arg-list (e.g. public.foo(uuid)), so alter\s+function[\s'\s%]*set\s+search_path will never match a real ALTER FUNCTION ... SET search_path statement. Since this is a pattern-not-regex ANDed with the other three, it has zero effect today — pattern 3 (line 253, [\s\S]{0,2000}) already provides the actual coverage for the "separate ALTER FUNCTION hardening" case. Remove line 254 or fix the intent if it was meant to allow something pattern 3 doesn't (e.g. quoted/schema-qualified names with no whitespace before SET).
♻️ Suggested fix
patterns:
- pattern-regex: "(?i)security\\s+definer"
- pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,8000}set\\s+search_path"
- pattern-not-regex: "(?is)alter\\s+function[\\s\\S]{0,2000}set\\s+search_path"
- - pattern-not-regex: "(?is)alter\\s+function[\\s'\\s%]*set\\s+search_path"📝 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
| patterns: | |
| - pattern: SECURITY DEFINER | |
| - pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,500}set\\s+search_path" | |
| - pattern-regex: "(?i)security\\s+definer" | |
| - pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,8000}set\\s+search_path" | |
| - pattern-not-regex: "(?is)alter\\s+function[\\s\\S]{0,2000}set\\s+search_path" | |
| - pattern-not-regex: "(?is)alter\\s+function[\\s'\\s%]*set\\s+search_path" | |
| patterns: | |
| - pattern-regex: "(?i)security\\s+definer" | |
| - pattern-not-regex: "(?is)security\\s+definer[\\s\\S]{0,8000}set\\s+search_path" | |
| - pattern-not-regex: "(?is)alter\\s+function[\\s\\S]{0,2000}set\\s+search_path" |
🤖 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 @.coderabbit/semgrep/helmv3.yml around lines 250 - 254, The extra pattern in
helmv3-security-definer-without-search-path is dead because the regexp after
alter\s+function cannot match real function signatures, so it never contributes
beyond the broader ALTER FUNCTION check. Remove the redundant pattern-not-regex
or rewrite it in the helmv3.yml rule so it correctly covers the intended ALTER
FUNCTION ... SET search_path cases, using the existing pattern-not-regex entries
around security definer and alter function as the reference point.
| const REPO_ROOT = resolve(import.meta.dirname, '..', '..'); | ||
| const SCRIPTS_DIR = join(REPO_ROOT, 'scripts'); | ||
|
|
||
| const HARDCODED_JWT = /eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9/; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
HARDCODED_JWT only catches one specific JWT header (HS256/JWT).
A token signed with a different algorithm (e.g. ES256, used by newer Supabase key formats) won't match this exact base64 header string and would slip through undetected. Consider reusing the more general JWT-shape pattern already established in this same PR's helmv3-hardcoded-supabase-credentials-py semgrep rule (eyJ[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}) for consistent, broader coverage.
🤖 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 `@scripts/__tests__/scripts-no-committed-secrets.test.mjs` at line 9, The
HARDCODED_JWT matcher in the secrets test is too narrow because it only matches
one HS256 header and can miss other valid JWTs such as ES256. Update the pattern
used in the test to the broader JWT-shape regex already used by the
helmv3-hardcoded-supabase-credentials-py semgrep rule, so the
scripts/no-committed-secrets check catches any three-part JWT-like token
consistently.
Comment on lines
+13
to
+27
| const BASEBALL_SEED_GLOB = /^seed-baseball-.*\.(mjs|ts)$/; | ||
|
|
||
| async function listSeedScripts(dir, acc = []) { | ||
| const entries = await readdir(dir, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| const full = join(dir, entry.name); | ||
| if (entry.isDirectory()) { | ||
| if (entry.name === '__tests__' || entry.name === 'node_modules') continue; | ||
| await listSeedScripts(full, acc); | ||
| } else if (BASEBALL_SEED_GLOB.test(entry.name)) { | ||
| acc.push(full); | ||
| } | ||
| } | ||
| return acc; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
BASEBALL_SEED_GLOB silently excludes scripts/seed-rini-baseball-demo.ts from the secret scan.
The regex ^seed-baseball-.*\.(mjs|ts)$ only matches filenames that start with seed-baseball-. seed-rini-baseball-demo.ts — modified by this very PR's seed-hardening cohort — starts with seed-rini-, so it's never collected by listSeedScripts and never scanned for the hardcoded JWT/prod-URL patterns this test exists to catch.
🔒 Suggested fix
-const BASEBALL_SEED_GLOB = /^seed-baseball-.*\.(mjs|ts)$/;
+const BASEBALL_SEED_GLOB = /^seed-.*baseball.*\.(mjs|ts)$/;📝 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 BASEBALL_SEED_GLOB = /^seed-baseball-.*\.(mjs|ts)$/; | |
| async function listSeedScripts(dir, acc = []) { | |
| const entries = await readdir(dir, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const full = join(dir, entry.name); | |
| if (entry.isDirectory()) { | |
| if (entry.name === '__tests__' || entry.name === 'node_modules') continue; | |
| await listSeedScripts(full, acc); | |
| } else if (BASEBALL_SEED_GLOB.test(entry.name)) { | |
| acc.push(full); | |
| } | |
| } | |
| return acc; | |
| } | |
| const BASEBALL_SEED_GLOB = /^seed-.*baseball.*\.(mjs|ts)$/; | |
| async function listSeedScripts(dir, acc = []) { | |
| const entries = await readdir(dir, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const full = join(dir, entry.name); | |
| if (entry.isDirectory()) { | |
| if (entry.name === '__tests__' || entry.name === 'node_modules') continue; | |
| await listSeedScripts(full, acc); | |
| } else if (BASEBALL_SEED_GLOB.test(entry.name)) { | |
| acc.push(full); | |
| } | |
| } | |
| return acc; | |
| } |
🤖 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 `@scripts/__tests__/scripts-no-committed-secrets.test.mjs` around lines 13 -
27, The secret-scan test is missing scripts/seed-rini-baseball-demo.ts because
BASEBALL_SEED_GLOB in listSeedScripts only matches names starting with
seed-baseball-. Update the seed filename filter so it also includes the rini
baseball demo seed script, and keep the traversal logic in listSeedScripts
unchanged so that all intended seed files are collected and scanned.
Comment on lines
+49
to
+83
| async function signIn(email, password) { | ||
| if (!email || !password) return null; | ||
| const res = await fetch(`${BASE_URL}/baseball/login`, { redirect: 'manual' }); | ||
| void res; | ||
| const loginPage = await fetch(`${BASE_URL}/api/auth/login`, { | ||
| method: 'POST', | ||
| headers: { 'content-type': 'application/json' }, | ||
| body: JSON.stringify({ email, password }), | ||
| }).catch(() => null); | ||
| if (!loginPage || !loginPage.ok) { | ||
| return null; | ||
| } | ||
| const setCookie = loginPage.headers.getSetCookie?.() ?? []; | ||
| return setCookie.join('; '); | ||
| } | ||
|
|
||
| async function crawlRoute(path, cookie) { | ||
| const res = await fetch(`${BASE_URL}${path}`, { | ||
| headers: cookie ? { cookie } : {}, | ||
| redirect: 'manual', | ||
| }); | ||
| const status = res.status; | ||
| const location = res.headers.get('location') || ''; | ||
| const bouncedToLogin = location.includes('/login') || location.includes('/signup'); | ||
| return { path, status, bouncedToLogin }; | ||
| } | ||
|
|
||
| async function main() { | ||
| const routes = loadNavRoutes(); | ||
| const cookie = await signIn(COACH.email, COACH.password); | ||
|
|
||
| if (!cookie) { | ||
| console.log('No baseball coach credentials — skipping authenticated crawl (set E2E_BASEBALL_COACH_EMAIL/PASSWORD).'); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== /api/auth/login route ==="
fd -p "src/app/api/auth/login"
rg -n "auth/login" src/app/api -g "*.ts"
echo "=== baseball login implementation ==="
rg -n "signInWithPassword" src/app/baseball -g "*.ts" -g "*.tsx"Repository: njrini99-code/helmv3
Length of output: 1854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== scripts/route-crawler-baseball.mjs ==="
nl -ba scripts/route-crawler-baseball.mjs | sed -n '1,140p'
echo
echo "=== e2e/baseball-phase1.spec.ts tryLogin ==="
if [ -f e2e/baseball-phase1.spec.ts ]; then
nl -ba e2e/baseball-phase1.spec.ts | sed -n '1,220p'
else
echo "missing e2e/baseball-phase1.spec.ts"
fi
echo
echo "=== baseball auth action context ==="
nl -ba src/app/baseball/actions/auth.ts | sed -n '80,140p'Repository: njrini99-code/helmv3
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== scripts/route-crawler-baseball.mjs ==="
sed -n '1,140p' scripts/route-crawler-baseball.mjs | cat -n
echo
echo "=== e2e/baseball-phase1.spec.ts tryLogin ==="
if [ -f e2e/baseball-phase1.spec.ts ]; then
sed -n '1,240p' e2e/baseball-phase1.spec.ts | cat -n
else
echo "missing e2e/baseball-phase1.spec.ts"
fi
echo
echo "=== baseball auth action context ==="
sed -n '80,140p' src/app/baseball/actions/auth.ts | cat -n
echo
echo "=== auth route search ==="
rg -n "auth/login|signInWithPassword|tryLogin" src/app e2e -g "*.ts" -g "*.tsx"Repository: njrini99-code/helmv3
Length of output: 20006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== baseball login page / form wiring ==="
rg -n "baseball/login|signInWithPassword|action=\{|useActionState|login" src/app/baseball -g "*.tsx" -g "*.ts"
echo
echo "=== baseball login page files ==="
fd -p "src/app/baseball" | rg "/login|/auth|page\.tsx$|page\.ts$"Repository: njrini99-code/helmv3
Length of output: 19783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== baseball login page source ==="
sed -n '1,220p' src/app/baseball/(auth)/login/page.tsx | cat -n
echo
echo "=== baseball login layout / action references ==="
rg -n "loginAction|useActionState|form action|redirect\\(|returnTo" src/app/baseball/\(auth\)/login -g "*.tsx" -g "*.ts"Repository: njrini99-code/helmv3
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== baseball login page source ==="
sed -n '1,220p' 'src/app/baseball/(auth)/login/page.tsx' | cat -n
echo
echo "=== baseball login layout / action references ==="
rg -n "loginAction|useActionState|form action|redirect\\(|returnTo" 'src/app/baseball/(auth)/login' -g "*.tsx" -g "*.ts"Repository: njrini99-code/helmv3
Length of output: 11759
Fail closed when baseball auth bootstrap fails scripts/route-crawler-baseball.mjs:49-83 — signIn() returns null for both missing credentials and any login error, so main() exits 0 and skips the crawl. There is no /api/auth/login route here; the login entry point is /baseball/login (src/app/baseball/(auth)/login/page.tsx:8,140). Remove the unused GET to /baseball/login and make auth failure exit 1 so CI cannot pass without crawling anything.
🤖 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 `@scripts/route-crawler-baseball.mjs` around lines 49 - 83, The baseball
crawler auth flow is treating real login failures the same as missing
credentials, letting `main()` exit 0 and skip the crawl. In `signIn()` and
`main()` from `scripts/route-crawler-baseball.mjs`, remove the unused GET to
`/baseball/login`, use the actual login entry point at `/baseball/login`, and
make authentication errors return a non-null failure signal so the script exits
1 when login/bootstrap fails instead of silently passing CI.
Comment on lines
+17
to
+29
| vi.mock('@/lib/supabase/server', () => ({ | ||
| createClient: vi.fn(async () => ({ | ||
| auth: { getUser, signInWithPassword, updateUser }, | ||
| })), | ||
| })); | ||
| vi.mock('next/cache', () => ({ revalidatePath: vi.fn() })); | ||
| vi.mock('@/lib/auth/supabase-rate-limit', () => ({ | ||
| checkRateLimit, | ||
| resetRateLimit, | ||
| RATE_LIMITS: { PASSWORD_CHANGE: { maxAttempts: 5, windowMs: 900000 } }, | ||
| formatTimeRemaining: vi.fn(() => '5m'), | ||
| })); | ||
| vi.mock('@/lib/auth/password-validation', () => ({ validatePassword })); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mock logServerError in this action test.
The wrong-password path at Lines 58-68 calls changePasswordAction, which logs through logServerError before returning the sanitized message. Without mocking @/lib/server-error-logger, this unit test can hit Sentry/admin-event plumbing instead of staying hermetic.
Proposed fix
vi.mock('`@/lib/auth/password-validation`', () => ({ validatePassword }));
+vi.mock('`@/lib/server-error-logger`', () => ({
+ logServerError: vi.fn(),
+}));Also applies to: 58-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/__tests__/change-password.test.ts` around lines 17 -
29, The change-password action test is missing a mock for the server error
logger, so the wrong-password path in changePasswordAction can exercise real
Sentry/admin-event plumbing instead of staying isolated. Add a vi.mock for
"`@/lib/server-error-logger`" alongside the existing mocks in
change-password.test.ts, and ensure the mocked logServerError is used when the
wrong-password branch runs so the test remains hermetic.
Comment on lines
+575
to
+582
| export const generateTeamInviteCode = withBaseballAction( | ||
| 'generateTeamInviteCode', | ||
| { | ||
| featureArea: 'baseball-settings', | ||
| requiredCapability: 'can_manage_settings', | ||
| teamFrom: (teamId: string) => teamId, | ||
| }, | ||
| async (ctx, teamId: string): Promise<TeamInviteResult> => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C3 '\b(generateTeamInviteCode|regenerateTeamInviteCode|getCoachTeamForManagement)\s*\(' -g '!src/app/baseball/actions/teams.ts'Repository: njrini99-code/helmv3
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Feature context for the reviewed file
npm run knowledge:map -- --files src/app/baseball/actions/teams.ts
npm run knowledge:context -- --files src/app/baseball/actions/teams.ts --task "review error handling contract for team invite actions"
# Inspect the relevant file region
wc -l src/app/baseball/actions/teams.ts
sed -n '520,760p' src/app/baseball/actions/teams.ts
# Find all callers/usages of the exported actions
rg -n -C 2 '\b(generateTeamInviteCode|regenerateTeamInviteCode|getCoachTeamForManagement)\b' srcRepository: njrini99-code/helmv3
Length of output: 12165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the baseball action wrapper and nearby action patterns
rg -n -C 4 'function withBaseballAction|const withBaseballAction|export function withBaseballAction|BaseballUnauthorizedError|BaseballNoActiveTeamError|BaseballCapabilityError' src/app/baseball src/lib -g '!**/*.map'
# Inspect the caller error handling around the invite actions
sed -n '80,140p' src/components/baseball/settings/TeamSettingsClient.tsx
sed -n '50,120p' src/components/baseball/command-center/BaseballInviteButton.tsx
# Find any other direct callers
rg -n -C 2 '\b(generateTeamInviteCode|regenerateTeamInviteCode|getCoachTeamForManagement)\b' src/components src/appRepository: njrini99-code/helmv3
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect wrapper implementation and action/caller behavior.
rg -n -C 5 'withBaseballAction|BaseballUnauthorizedError|BaseballNoActiveTeamError|BaseballCapabilityError' src/app/baseball src/lib -g '!**/*.map'
sed -n '80,140p' src/components/baseball/settings/TeamSettingsClient.tsx
sed -n '50,120p' src/components/baseball/command-center/BaseballInviteButton.tsx
rg -n -C 2 '\b(generateTeamInviteCode|regenerateTeamInviteCode|getCoachTeamForManagement)\b' src/components src/appRepository: njrini99-code/helmv3
Length of output: 50377
Catch invite-action rejections in src/components/baseball/settings/TeamSettingsClient.tsx:105-113
generateTeamInviteCode / regenerateTeamInviteCode can throw BaseballUnauthorizedError, BaseballNoActiveTeamError, or BaseballCapabilityError before returning TeamInviteResult. This path has no try/catch, so permission-denied skips the error toast; wrap the await and map rejections into the existing error state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/teams.ts` around lines 575 - 582, The invite action
call path in TeamSettingsClient needs error handling because
generateTeamInviteCode/regenerateTeamInviteCode can reject with
BaseballUnauthorizedError, BaseballNoActiveTeamError, or BaseballCapabilityError
before returning TeamInviteResult. Wrap the await in the TeamSettingsClient
invite handler with try/catch, and on failure route the error into the existing
error state/toast flow instead of letting it bypass the UI feedback.
Comment on lines
157
to
+166
| try { | ||
| return await createItineraryAction(teamId, data); | ||
| } catch (err) { | ||
| if (err instanceof z.ZodError) { | ||
| return { success: false as const, error: err.issues[0]?.message || 'Invalid data.' }; | ||
| } | ||
| await logServerError(`[Baseball Travel] Unexpected error: ${err instanceof Error ? err.message : String(err)}`, { action: 'travel.createItinerary' }); | ||
| return mapTravelActionError(err); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
z.ZodError handling is dead code — validation messages are lost.
createItinerarySchema.parse(...) runs inside createItineraryAction (line 180), so a ZodError is thrown inside withBaseballAction, which converts any non-typed throw into BaseballActionError. By the time it reaches this catch, err instanceof z.ZodError is always false, so the user gets the generic mapTravelActionError message instead of "Event name is required". Same in updateItinerary (226-234) and addExpense (346-355). Use safeParse inside the action and return the field message directly (a success:false return is not re-wrapped by the wrapper).
🔧 Validate with safeParse inside the action
- const validated = createItinerarySchema.parse({
+ const parsed = createItinerarySchema.safeParse({
team_id: teamId,
event_name: data.event_name,
departure_date: data.departure_date || null,
return_date: data.return_date || null,
location: data.location || null,
accommodation: data.accommodation || null,
transportation: data.transportation || null,
notes: data.notes || null,
- });
+ });
+ if (!parsed.success) {
+ return { success: false as const, error: parsed.error.issues[0]?.message || 'Invalid data.' };
+ }
+ const validated = parsed.data;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/travel.ts` around lines 157 - 166, The `z.ZodError`
branch in `createItineraryAction` is unreachable because `withBaseballAction`
wraps thrown validation errors before they reach this `catch`, so the
field-level message is lost. Move validation in `createItineraryAction`,
`updateItinerary`, and `addExpense` to `safeParse`/`safeParseAsync` before any
throw, and return the specific schema error message directly as a `success:
false` result so it bypasses `BaseballActionError` wrapping. Keep the existing
`logServerError`/`mapTravelActionError` path only for unexpected non-validation
failures.
Comment on lines
+48
to
+56
| if (error instanceof z.ZodError) { | ||
| const firstError = error.issues?.[0]; | ||
| return { success: false, error: firstError?.message || 'Invalid input data' }; | ||
| } | ||
| if (error instanceof Error) { | ||
| return { success: false, error: error.message }; | ||
| } | ||
| return { success: false, error: 'An unexpected error occurred' }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
z.ZodError branch in mapWatchlistActionError is unreachable.
WatchlistSchemas.*.parse(...) runs inside the wrapped actions (e.g. 246, 345, 403), so a ZodError is caught and re-thrown as BaseballActionError by withBaseballAction. The public wrappers therefore never see a z.ZodError, and this branch (48-51) is dead — invalid input surfaces the generic "Could not complete the watchlist action." instead of the schema's field message. Switch the in-action .parse() calls to safeParse and return the issue message (a success:false return is not re-wrapped), then this branch can be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/watchlist.ts` around lines 48 - 56, The z.ZodError
handling in mapWatchlistActionError is dead because withBaseballAction re-wraps
parse failures before the public wrapper sees them. Update the watchlist actions
that call WatchlistSchemas.*.parse(...) to use safeParse instead and return a
success:false result with the schema issue message directly, so invalid input is
surfaced from the action and the z.ZodError branch in mapWatchlistActionError
can be removed.
Comment on lines
6
to
11
| "git": { | ||
| "deploymentEnabled": true | ||
| "deploymentEnabled": { | ||
| "*": false, | ||
| "main": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Project convention prefers vercel.ts over vercel.json.
As per path instructions, "Prefer vercel.ts over vercel.json (the project is on Vercel...)." This existing file predates that convention, so not blocking, but worth migrating when this file is next touched substantially.
🤖 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 `@vercel.json` around lines 6 - 11, The Vercel configuration currently lives in
vercel.json, but the project convention prefers vercel.ts. When this config is
next touched, migrate the existing deploymentEnabled settings into the vercel.ts
configuration entry point and remove the JSON-based config so the Vercel setup
follows the repository standard.
Source: Path instructions
Comment on lines
+7
to
+10
| "deploymentEnabled": { | ||
| "*": false, | ||
| "main": true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vercel.json =="
nl -ba vercel.json | sed -n '1,80p'
echo
echo "== lighthouse-preview references =="
rg -n "lighthouse-preview|wait-for-vercel-preview|NO_DEPLOYMENT_GRACE_S|deploymentEnabled" .circleci vercel.json . -g '!node_modules' -g '!dist' -g '!build' || true
echo
echo "== circleci config excerpt =="
nl -ba .circleci/config.yml | sed -n '1,260p'
echo
echo "== wait-for-vercel-preview script =="
nl -ba .circleci/scripts/wait-for-vercel-preview.sh | sed -n '1,260p'Repository: njrini99-code/helmv3
Length of output: 216
🌐 Web query:
Vercel git.deploymentEnabled branch rules any matching rule true unspecified branch default true documentation
💡 Result:
In Vercel project configuration, the git.deploymentEnabled property controls whether commits to specific Git branches trigger automatic deployments [1]. Key behavioral rules for git.deploymentEnabled: 1. Default Behavior: By default, any branch that is not explicitly specified is set to true, meaning automatic deployments are enabled for all branches unless otherwise configured [1][2]. 2. Global Disable: You can disable automatic deployments for all branches by setting the property to false [1]. 3. Branch-Specific Rules: You can provide an object where the keys are branch names or glob patterns (using minimatch syntax) and the values are booleans (true or false) [1][3]. 4. Matching Multiple Rules: If a branch matches multiple rules, a deployment will occur if at least one of those matching rules is set to true [1]. Configuration Example: To prevent automatic deployments for specific branches, such as those starting with "internal-", you can use the following configuration in your vercel.json: { "git": { "deploymentEnabled": { "internal-": false } } } If a branch matches multiple rules with conflicting boolean values (e.g., "experiment-" is false, but "*-dev" is true), the presence of any true rule will result in an enabled deployment for that branch [1].
Citations:
- 1: https://vercel.com/docs/project-configuration/git-configuration
- 2: https://catalog.lintel.tools/schemas/schemastore/vercel/
- 3: https://vercel.com/changelog/granular-branch-matching-for-git-configuration-in-vercel-json
🏁 Script executed:
python3 - <<'PY'
import json, sys, os, pathlib, re
# placeholder read-only verifier scaffold if results are available in repo context only
PYRepository: njrini99-code/helmv3
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused, read-only branch-rule sanity check against the repo's Vercel config.
python3 - <<'PY'
import json, pathlib, re
vercel = json.loads(pathlib.Path('vercel.json').read_text())
rules = vercel.get('git', {}).get('deploymentEnabled', {})
print("deploymentEnabled:", rules)
for branch in ['main', 'feature/x', 'pr-123']:
matches = []
for pat, enabled in rules.items():
if pat == '*':
matches.append((pat, enabled))
elif pat == branch:
matches.append((pat, enabled))
print(branch, matches, "=>", any(enabled for _, enabled in matches))
PYRepository: njrini99-code/helmv3
Length of output: 313
vercel.json:7-10 blocks preview deployments for every non-main branch
"deploymentEnabled": { "*": false, "main": true } disables all PR/feature branches, so the new Lighthouse-preview flow can’t get a Vercel preview URL for commit-based checks. If the goal is to cut noise, use explicit branch exclusions instead of a blanket wildcard disable.
Fix
"git": {
"deploymentEnabled": {
- "*": false,
- "main": true
+ "main": true,
+ "*": true
}
},📝 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
| "deploymentEnabled": { | |
| "*": false, | |
| "main": true | |
| } | |
| "deploymentEnabled": { | |
| "main": true, | |
| "*": true | |
| } |
🤖 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 `@vercel.json` around lines 7 - 10, The deploymentEnabled rule is disabling
preview deployments for all non-main branches, which prevents the Lighthouse
preview flow from getting Vercel preview URLs. Update the deploymentEnabled
configuration in vercel.json to avoid the blanket wildcard disable and instead
use explicit branch exclusions or a more targeted allowlist, keeping main
enabled while allowing preview branches needed for commit-based checks.
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.
Summary
Two waves of parallel codebase audits of BaseballHelm to find concrete, reproducible bugs and UI/UX problems — distinct from the architectural/tech-debt work already tracked in issues #367-#418.
STILL_VALID), plus 4 new-area audits: video/lineups/lifting, scout-packets/decision-room/compare, roles-permissions/program-settings, coach-notes/AI-governance/academics.89 distinct findings total, each backed by file/line evidence and cross-checked against open issues and prior-wave findings to avoid duplicates (one near-duplicate caught and dropped during consolidation).
Why this is docs/tooling, not filed issues
The agent running this audit only has read-only GitHub CLI access (per its tool policy) and there's no issue-creation MCP configured in this workspace, so the findings can't be filed directly from here. Instead:
scripts/baseballhelm-bug-audit/issues.mjs— all 89 findings as{ title, labels, body }, written in this repo's existing issue style (Problem / Evidence / Why it matters / Acceptance criteria — matches BaseballHelm: fix player Calendar team resolution #368's format).scripts/baseballhelm-bug-audit/create-issues.mjs— a runner that callsgh issue createfor each one, using your own authenticatedghaccess:docs/BASEBALLHELM_BUG_AUDIT_2026-06-30.md— readable summary, area breakdown, and caveats.Notable highlights (wave 2 additions)
approveAiOutputcan promote an AI signal withheld forplayer_visible_disabledto player-facing visibility anyway, bypassing the AI governance toggle.sourceRefs, and several of its read-models return entirely wrong field names (title/rationalevs the UI'slabel/detail), so meeting items render blank.Caveats