BaseballHelm coherence: one shell, capped nav IA, one Lift Lab, data honesty#792
Merged
Merged
Conversation
…ell, capped nav IA, one lift lab Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…te dead (coach-dashboard) route group Coherence Ruling 1 (docs/baseball/COHERENCE_RULING_2026-07-08.md): NEXT_PUBLIC_REDESIGN=true is what prod serves, so the legacy shell fork in (dashboard)/layout.tsx and (player-dashboard)/player/layout.tsx was pure regression risk. Both now mount BaseballFairwayShell unconditionally; the team-context DashboardSessionGuard is unchanged. Deletes src/app/baseball/(coach-dashboard)/ entirely (confirmed dead: only layout.tsx/template.tsx/error.tsx, zero page.tsx). Updates stale BaseballShellLayout references in comments/test descriptions to BaseballFairwayShell.
…, strip baseball nav from shared sidebar Both files had zero real importers once the (dashboard)/(player-dashboard) layouts render BaseballFairwayShell unconditionally (previous commit) — deleted per Ruling 1/5. src/components/layout/sidebar.tsx is shared with golf: removed the 5 legacy baseball nav arrays (collegeTeamNav/hsCoachTeamNav/jucoTeamNav/showcaseOrgNav/playerTeamNav), buildCondensedBaseballNavigation(), the showcase org/team TeamSwitcher branch, and every other baseball-only branch. The isGolf branch (golfCoachNav, dashboardHref, EXACT_MATCH_HREFS golf entry, coachSecondaryNav/playerSecondaryNav fallback, isHubItemActive) is byte-identical to before — golf renders the exact same values it did prior to this change. Note: BaseballHelm no longer renders this Sidebar component at all (it only ever had one consumer, the now-deleted BaseballDashboardShell) — GolfHelm's real rendered sidebar is the separate GolfSidebar.tsx via GolfDashboardShell, not this file. This component is consequently unreferenced by any route today; flagged for the commander as a follow-up deletion candidate, not removed here since it falls outside this task's named scope and the file is marked shared/untouchable without further authorization.
…dar/documents/messages pages Calendar, Documents, and Messages pages had their own local isRedesignEnabled() forks rendering a legacy pre-Fairway UI. Since the shell fork is gone (BaseballFairwayShell is now the only shell), these pages always take the Fairway branch — deleted the legacy JSX and now-unused imports (Link, BaseballCalendarWrapper, EVENT_TYPE_CONFIG, Card/CardContent/Input/Select/Checkbox/DocumentCard/cn, the messages skeleton block). practice/page.tsx already had no fork (comment-only precedent reference, untouched).
…ubtabs (Ruling 2) Implements the coach hub/subtab table from docs/baseball/COHERENCE_RULING_2026-07-08.md Ruling 2. Mechanism: nav-registry.ts gets a `foldedUnder` field (a registry entry marks itself as reachable only from a parent landing page instead of its own hub-tab slot, but keeps its registry row so command palette + deep links still work) and a `hubTabLabel` override (lets a subtab read differently from the entry's canonical label, e.g. "Performance" reads "Training" under Development). hub-definitions.ts's `hubEntries()` excludes folded entries from the rendered strip. Regrouped: Announcements moves from Team to a new real Messages hub (Messages · Announcements, replacing the old bare flat link); Practice Planner/Effectiveness move from Stats & Performance to Team (fold into the new Operations landing); Performance moves from Stats & Performance to Development (folds in as "Training"); Import Center folds into Stats Center; Interest folds into Pipeline; Watchlist/Compare/Saved Comparisons/Scout Packets/Camps fold into a new Scouting landing; Program Info/Staff Settings/Program Settings fold into the existing Settings landing; Teams/ Events fold into Organization. Deleted the 9-item SETTINGS_* hub-tab splice and the 4-item PERFORMANCE_* splice outright (dead code, not layered over) since Performance's own page already links Programs/Live/Builder/Groups. Command Center's nav-visible label renames to "Overview" (breadcrumbs, hub subtab, command palette, mobile nav) so "Dashboard" never has two names in the UI; the route/page masthead keep "Command Center". BaseballFairwayShell.tsx: Messages now flows through the generic COACH_HUB_ORDER loop (with its unread badge threaded in by hub id) instead of a hand-injected flat item; showcase org/team rails route their Messages item through the same hub-aware path so showcase coaches don't lose Announcements reachability. Mobile bottom nav trimmed to the top 3 destinations per role (Ruling 2 item 8) — the top-bar hamburger (already bridged to the same drawer) is the "+ More". /baseball/dashboard/practice now server-redirects players to the canonical /baseball/player/practice surface (item 7); coach/staff rendering unchanged. nav-manifest.ts and route-contract.ts extended to track the new Messages hub tabs for completeness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
New card-grid landing pages that keep the folded destinations reachable in ≤2 clicks now that Team and Recruiting cap at 3 rendered subtabs: - /baseball/dashboard/operations (Team hub): Documents, Travel, Practice Planner, Practice Effectiveness. - /baseball/dashboard/scouting (Recruiting hub): Watchlist, Compare Players, Saved Comparisons, Scout Packets, Camps. Both are server components (no hooks) built from the Living Annual kit (SectionMasthead/PaperCard/Eyebrow). Neither re-declares capability or program-type gating — each card is resolved via getBaseballNavEntry + isBaseballNavEntryVisible against the SAME registry entry the sidebar and command palette use, so a coach without a capability simply doesn't see that card, exactly like they wouldn't see it in the sidebar. Operations uses the green "team" ink lane; Scouting uses the clay "pursuit" ink lane (recruiting). Also adds the Stats Center masthead's persistent header-level actions (Upload, Import Center) per Ruling 2 item 2 — Upload + Import Center are now CTAs on the Stats Center page instead of their own hub-tab slots; the existing empty-state "Import a box score" CTA is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…e (Ruling 2) The card-grid landing at /dashboard/settings stays the single settings nav surface (its own 13-tab strip is gone now that Management caps at 3 subtabs: Decision Room · Settings · Organization). It was mounting the legacy <Header> component on top of the shell's own top bar, duplicating breadcrumbs and the mobile-menu affordance; replaced with a plain title block. Verified the card grid still covers every fold target (Program Info/Settings, Staff, Team, Season, Philosophy, Roles, Permissions, Imports, Integrations, Audit) — nothing orphaned by the splice removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…(Ruling 2)
nav-manifest.test.ts: extends the "every coach/both entry declares exactly
one hub" test to the new 8-hub set (adds 'messages'), and adds a permanent
lock (item 10 of the A2 task) asserting:
(b) every coach hub renders ≤3 subtabs — both the raw arrays and a
fully-capable head coach in every program type.
(c) every player hub renders ≤3 subtabs.
(d) every foldedUnder-marked registry entry still resolves to a hub via
resolveActiveHub (command palette + deep links stay live even without
their own hub-tab slot).
resolve-active-hub.test.ts: rewritten for the restructured hubs — Training
(performance) now resolves under Development instead of Stats & Performance;
Stats Center's Season/Upload/Import Center folds; Team's Operations folds
(Documents/Travel/Practice Planner/Practice Effectiveness); the new Messages
hub (Announcements moved off Team); Management's single Settings tab
(replacing the deleted 9-tab splice) plus Organization's Teams/Events folds;
Recruiting's Scouting folds and Pipeline's Interest fold. Behavior
intentionally changed per the ruling, so these tests were updated WITH the
change rather than left asserting the old structure.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…listic seed event times Readiness page dated check-ins in server UTC while the gate compared team-local (evening US submissions mis-dated, gate kept nagging). Command Center day + day bounds now team-local via the same contract-day helpers player-today already uses. Seed script events get real times (practice 15:30-17:30, game 13:00-16:00, meeting 12:00-13:00 ET) instead of the script's own wall-clock with zero duration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…IT V, unclip KPI numerals, slash-line backfill Academics: is_eligible is boolean|null end-to-end; no record renders neutral 'Not on file', red Ineligible reserved for a real determination. Roster: EXIT V column removed (avg_exit_velocity doesn't exist in the live schema and has no write path). RuledStatLine: drop leading-none stacked over the text-ink token's own line-height (numeral clipping). New idempotent backfill script populated career_obp/slg/ops for 20/22 aggregate rows in prod using the app's tested computeCareerSlashLine logic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…y real names buildBreadcrumbs extracted to a pure module with an id-shape guard (UUID/numeric segments fall back to the owning label, never title-cased). New BreadcrumbLabel override channel: players/[id] shows the player's name, stats/games/[gameId] the opponent, dev-plans/[id] the plan. 17 unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
Null end_time renders start+1h (display only), events query bounded (90d back / 365d fwd, limit 500 under the PostgREST cap), count badges pluralize. Swallowed supabase insert errors in createBaseballEvent/createGame/box-score paths are now checked and surfaced (ActionResult.warning). E2E box-score spec deletes its own created games+events in teardown (prod was already purged of 160 junk rows). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…cal helm_lifting stack All six lift/performance routes (programs, program editor, groups, live weight room, player lift home, player session) repointed at src/components/lifting/*. Six legacy duplicates in components/baseball/performance/ deleted (GolfHelm palette, legacy baseball_lift_* writes). PlayerLiftToday comment corrected to helm_lifting_*. Coach edit access preserved via helm_lifting_org_viewers bridge migration (9/10 coaches; 10th has no org). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…lumns (applied to prod) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…cold-URL bounce Deleted: duplicate players/[id]/profile page (canonical = players/[id]), orphaned sidebar.tsx + team-switcher.tsx (zero importers — golf renders GolfSidebar), knip-confirmed orphans (MatchScoreBadge, match-calculator, dashboard-types), stale baseballhelm-* local scripts. Dev-plan page is now a server component (requireBaseballPlayerRoute) wrapping DevPlanClient — no more guard-race bounce to Today on hard navigation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…ber empty states, dev-plan chrome, warning toasts Removed duplicate page-level Header mounts (shell owns the masthead). All empty/error states on team-ops + player surfaces route through the kit (EmptyIssue/EditorsLetter) — ReadModelStateNotice de-ambered for its 6 call sites. DevPlanClient rebuilt on kit primitives with ≥44px touch targets. createBaseballEvent partial-failure warnings surface as toasts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…-portal revalidation, read-only dynamic groups Command Center week window now team-local AND respects forDate (was bare new Date()). Player snapshot cards fall back to career obp/slg/ops like avg. Lifting mutations revalidate the baseball routes that now render them (Wave C), not just /lifting. StrengthGroupsClient renders rule-managed dynamic groups first-class read-only (badge + rule summary, destructive affordances disabled) — both existing prod groups are dynamic. +15 unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
|
Too many files changed for review. ( Bypass the limit by tagging |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Updates to Preview Branch (fix/baseballhelm-coherence-2026-07-08) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
Summary by CodeRabbit
WalkthroughThis PR applies the BaseballHelm coherence ruling: it unifies the shell and navigation model, adds pathname-based breadcrumb labels, migrates several dashboard surfaces to Living Annual components, tightens date and data correctness, and repoints baseball lift/performance flows onto canonical ChangesBaseballHelm Coherence Ruling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client as BaseballCalendarWrapper
participant Action as createBaseballEventAction
participant DB as Supabase
Client->>Action: createEvent(payload)
Action->>DB: insert baseball_events
Action->>DB: insert baseball_event_attendance
DB-->>Action: attendance insert error
Action->>DB: insert baseball_games (if game/scrimmage)
Action-->>Client: success + warning
Client->>Client: toast.warning(warning)
sequenceDiagram
participant Page as lift/page.tsx
participant Ctx as resolvePlayerLiftAthleteContext
participant DB as Supabase
participant Client as PlayerLiftHomeClient
Page->>Ctx: resolve athlete context
Ctx-->>Page: organizationId, athleteId, teamId
Page->>DB: fetch helm_lifting_sessions
DB-->>Page: upcoming and recent sessions
Page->>Client: render session data
Possibly related PRs
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 🔧 ESLint
e2e/baseball-box-score.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. scripts/backfill-baseball-slash-lines.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. scripts/e2e-supabase-admin.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
PR Summary by QodoBaseballHelm coherence: unify shell/nav, one Lifting Lab, and data-honesty fixes
AI Description
Diagram
High-Level Assessment
Files changed (100)
|
…inventory docs Stat-layer manifest grandfathers the command-center test fixture (mirrors the already-grandfathered read-model); RosterFairway comment drops the literal deprecated-table name; product-trust documents contract asserts the structural EmptyIssue empty path + kit copy source instead of the pre-redesign string; inventory docs regenerated (same content as auto-PR #784). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
97 rules 1. supabase as any in calendar
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+241
to
+243
| const { error: attendanceError } = await (supabase as any) | ||
| .from('baseball_event_attendance') | ||
| .insert(attendanceRecords); |
There was a problem hiding this comment.
1. supabase as any in calendar 📘 Rule violation ⚙ Maintainability
The server action uses an explicit any cast (supabase as any) for database inserts, bypassing type safety. This violates the project rule disallowing any and can mask incorrect payload shapes or query usage.
Agent Prompt
## Issue description
`src/app/baseball/actions/calendar.ts` introduces explicit `any` casts (`supabase as any`) for inserts, violating the `any` ban and weakening type safety.
## Issue Context
The action already imports `fromUntyped` and can avoid `any` by using typed helpers and/or narrow, explicit row payload types for `insert()` calls.
## Fix Focus Areas
- src/app/baseball/actions/calendar.ts[240-253]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+22
to
+28
| function makeQueryBuilder(finalResult: FinalResult) { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const builder: any = {}; | ||
| const passthrough = ['select', 'eq', 'is']; | ||
| for (const m of passthrough) { | ||
| builder[m] = vi.fn(() => builder); | ||
| } |
There was a problem hiding this comment.
2. builder: any in groups test 📘 Rule violation ⚙ Maintainability
A new lifting actions test declares const builder: any = {}, introducing an explicit any type.
This violates the rule disallowing any and reduces the test’s ability to catch shape mistakes in
the mocked query builder.
Agent Prompt
## Issue description
The new test uses an explicit `any` type (`const builder: any = {}`), violating the `any` ban.
## Issue Context
The builder can be typed as a minimal interface (methods returning itself) or as `Record<string, unknown>` plus explicit method signatures, avoiding `any` while keeping the mock flexible.
## Fix Focus Areas
- src/app/lifting/actions/__tests__/groups.test.ts[22-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+43
to
+77
| import { config as loadEnv } from 'dotenv'; | ||
| import { createClient, type SupabaseClient } from '@supabase/supabase-js'; | ||
| import { computeCareerSlashLine, type SlashLineStatRow } from '../src/lib/baseball/aggregates/career-slash-line'; | ||
|
|
||
| loadEnv({ path: '.env.local' }); | ||
|
|
||
| const DRY = !process.argv.includes('--confirm'); | ||
|
|
||
| interface AggregateRow { | ||
| player_id: string; | ||
| team_id: string; | ||
| career_avg: number | null; | ||
| career_obp: number | null; | ||
| career_slg: number | null; | ||
| career_ops: number | null; | ||
| } | ||
|
|
||
| function fmt(v: number | null): string { | ||
| return v == null ? '—' : v.toFixed(3); | ||
| } | ||
|
|
||
| function valuesEqual(a: number | null, b: number | null): boolean { | ||
| if (a == null && b == null) return true; | ||
| if (a == null || b == null) return false; | ||
| // Compare at the same 3-decimal precision the column/formula use. | ||
| return Math.abs(a - b) < 0.0005; | ||
| } | ||
|
|
||
| async function main() { | ||
| const url = (process.env.NEXT_PUBLIC_SUPABASE_URL ?? '').trim(); | ||
| const key = (process.env.SUPABASE_SERVICE_ROLE_KEY ?? '').trim(); | ||
| if (!url || !key) throw new Error('Missing NEXT_PUBLIC_SUPABASE_URL / SUPABASE_SERVICE_ROLE_KEY'); | ||
| const supabase: SupabaseClient = createClient(url, key, { | ||
| auth: { persistSession: false, autoRefreshToken: false }, | ||
| }); |
There was a problem hiding this comment.
3. Direct supabase client creation 📘 Rule violation ⌂ Architecture
New tooling code creates Supabase clients by importing createClient from @supabase/supabase-js instead of using the shared server-side factory. This violates the compliance requirement for centralized server-side Supabase client creation and increases risk of inconsistent auth/config handling.
Agent Prompt
## Issue description
Server-side Supabase clients are being instantiated directly from `@supabase/supabase-js` in new tooling/test code, rather than using the shared server-side factory.
## Issue Context
The compliance rule requires server-side DB access to obtain clients via `createClient` from `@/lib/supabase/server` (`await createClient()`). For service-role workflows, centralize the service-role construction behind the shared factory (or a wrapper in that module) so scripts/tests don’t reimplement client creation.
## Fix Focus Areas
- scripts/backfill-baseball-slash-lines.ts[43-77]
- e2e/baseball-box-score.spec.ts[1-64]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+7
to
+36
| INSERT INTO public.helm_lifting_org_viewers (organization_id, user_id, sport, can_edit, granted_by) | ||
| SELECT bc.organization_id, bc.user_id, 'baseball', true, 'migration:baseball-coach-lifting-bridge' | ||
| FROM public.baseball_coaches bc | ||
| WHERE bc.organization_id IS NOT NULL AND bc.user_id IS NOT NULL | ||
| ON CONFLICT (organization_id, user_id, sport) | ||
| DO UPDATE SET can_edit = true; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.bridge_baseball_coach_lifting_access() | ||
| RETURNS trigger | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path TO 'public', 'pg_temp' | ||
| AS $$ | ||
| BEGIN | ||
| IF NEW.organization_id IS NOT NULL AND NEW.user_id IS NOT NULL THEN | ||
| INSERT INTO public.helm_lifting_org_viewers (organization_id, user_id, sport, can_edit, granted_by) | ||
| VALUES (NEW.organization_id, NEW.user_id, 'baseball', true, 'trigger:baseball-coach-lifting-bridge') | ||
| ON CONFLICT (organization_id, user_id, sport) | ||
| DO UPDATE SET can_edit = true; | ||
| END IF; | ||
| RETURN NEW; | ||
| END; | ||
| $$; | ||
|
|
||
| REVOKE EXECUTE ON FUNCTION public.bridge_baseball_coach_lifting_access() FROM PUBLIC, anon, authenticated; | ||
|
|
||
| DROP TRIGGER IF EXISTS trg_bridge_baseball_coach_lifting ON public.baseball_coaches; | ||
| CREATE TRIGGER trg_bridge_baseball_coach_lifting | ||
| AFTER INSERT OR UPDATE OF organization_id, user_id ON public.baseball_coaches | ||
| FOR EACH ROW EXECUTE FUNCTION public.bridge_baseball_coach_lifting_access(); |
There was a problem hiding this comment.
5. Lifting access not revoked 🐞 Bug ⛨ Security
The new bridge_baseball_coach_lifting_access trigger only UPSERTs helm_lifting_org_viewers(can_edit=true) and never removes the old viewer row when a coach’s organization_id/user_id changes or is cleared, so users can retain edit access to orgs they no longer belong to. Because Lifting Lab edit authorization is derived from helm_lifting_org_viewers.can_edit, these orphaned rows become an authorization bug.
Agent Prompt
## Issue description
The migration introduces a SECURITY DEFINER trigger that grants `helm_lifting_org_viewers(can_edit=true)` for baseball coaches, but it never revokes access when a coach leaves/changes org (organization_id/user_id changed, nulled, or row deleted). This can leave stale `helm_lifting_org_viewers` rows that still grant edit access.
## Issue Context
- The bridge is implemented via an UPSERT-only trigger on `public.baseball_coaches`.
- Lifting Lab edit access is computed from `helm_lifting_org_viewers.can_edit`.
## Fix approach
1. Add a follow-up migration (preferred, since this migration was already applied to prod per comments) that:
- Replaces `public.bridge_baseball_coach_lifting_access()` to also handle updates where `OLD.organization_id/user_id` differ from `NEW` (delete the OLD mapping for sport='baseball').
- Adds an `AFTER DELETE` trigger on `public.baseball_coaches` to delete the corresponding `helm_lifting_org_viewers` row for sport='baseball'.
- Optionally limits deletions to rows where `granted_by` matches the bridge markers (e.g., `trigger:baseball-coach-lifting-bridge` / `migration:baseball-coach-lifting-bridge`) to avoid interfering with any future non-bridge grants.
2. Add a one-time cleanup statement to remove orphaned bridge-granted viewer rows that no longer have a matching `baseball_coaches` row for the same (user_id, organization_id).
## Fix Focus Areas
- supabase/migrations/20260708010000_bridge_baseball_coach_lifting_edit.sql[7-36]
- src/lib/lifting/access.ts[74-105]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+108
to
+123
| const { data: games } = await supabase | ||
| .from('baseball_games') | ||
| .select('id, event_id') | ||
| .in('opponent_name', createdOpponents); | ||
|
|
||
| const gameIds = (games ?? []).map((g) => g.id); | ||
| const eventIds = (games ?? []) | ||
| .map((g) => g.event_id) | ||
| .filter((id): id is string => Boolean(id)); | ||
|
|
||
| if (gameIds.length > 0) { | ||
| await supabase.from('baseball_games').delete().in('id', gameIds); | ||
| } | ||
| if (eventIds.length > 0) { | ||
| await supabase.from('baseball_events').delete().in('id', eventIds); | ||
| } |
There was a problem hiding this comment.
6. E2e teardown too broad 🐞 Bug ☼ Reliability
The new Playwright teardown deletes baseball_games/baseball_events using a service-role client by first selecting rows via opponent_name IN createdOpponents, which is not a guaranteed-unique identifier across teams/environments. In a shared database, this can delete unrelated real rows if an opponent_name collides or gets reused.
Agent Prompt
## Issue description
The E2E cleanup uses the service role key and finds rows to delete by matching `opponent_name`, which is not a safe uniqueness boundary. This creates a risk of deleting unrelated rows in shared DBs.
## Issue Context
- The test creates opponents via `E2E Created Opponent ${Date.now()}` and later deletes by opponent_name.
- The README documents CI uses `SUPABASE_SERVICE_ROLE_KEY` for seeding/cleanup.
## Fix approach
1. Track the created **game IDs** instead of opponent names:
- After clicking "Create Game" and waiting for the redirect, parse the `gameId` from `page.url()` and push it into `createdGameIds`.
2. Teardown should:
- Select `event_id` from `baseball_games` by `id IN createdGameIds` (optionally also `eq('team_id', SEEDED_TEAM_ID)` if available).
- Delete by `id` for both tables.
3. As a safety belt, consider adding an additional filter (team_id, created_by, or a dedicated `e2e_run_id` column if you have one) so cleanup is provably scoped.
## Fix Focus Areas
- e2e/baseball-box-score.spec.ts[59-123]
- e2e/baseball-box-score.spec.ts[145-158]
- e2e/README.md[181-210]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…-grep gate) The box-score spec's cleanup client referenced SUPABASE_SERVICE_ROLE_KEY directly, tripping helmv3-no-service-role-key (e2e/ is in scan scope; scripts/ is the sanctioned exempt home, same as seed-baseball-e2e.ts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/baseball/actions/calendar.ts (1)
240-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
fromUntyped()for the secondary baseball writes too.src/app/baseball/actions/calendar.ts:240-251already has the helper in scope, andsrc/lib/supabase/untyped.tsaccepts these table names, so the(supabase as any)casts andeslint-disable-next-linecomments are redundant.🤖 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/calendar.ts` around lines 240 - 267, The secondary baseball insert paths in the calendar action still cast `supabase` to `any` and disable linting, but `fromUntyped()` is already available in `calendar.ts` and should be used for both the `baseball_event_attendance` and `baseball_games` writes. Update those insert calls to use `fromUntyped()` directly, remove the redundant `eslint-disable-next-line` comments and `any` casts, and keep the existing error handling and revalidation behavior unchanged.
♻️ Duplicate comments (1)
src/app/baseball/(dashboard)/dashboard/operations/page.tsx (1)
1-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame duplication concern as
scouting/page.tsx.See the refactor comment on
src/app/baseball/(dashboard)/dashboard/scouting/page.tsx— this file is the second (of at least two, likely three counting/dashboard/settings) byte-for-byte-structural copy of the hub-landing-grid pattern. SameHubLandingGridextraction applies here; onlyOPERATIONS_CARD_IDS,ink="team", and the copy differ from the Scouting page.🤖 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/operations/page.tsx around lines 1 - 117, This page is duplicating the same hub-landing card-grid pattern seen in the Scouting landing, so extract the shared rendering into the existing HubLandingGrid-style component or helper and reuse it here. Keep Operations-specific data only in OPERATIONS_CARD_IDS, the masthead copy, and the team ink/theme; move the shared requireBaseballCoachRoute, getBaseballNavContext, getBaseballNavEntry, isBaseballNavEntryVisible, and cards-to-PaperCard/Link rendering logic into the common abstraction. Use OperationsPage and the shared grid component to locate the split points cleanly.
🤖 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 `@e2e/baseball-box-score.spec.ts`:
- Around line 95-119: Teardown is still identifying rows by opponent_name
instead of the created game’s primary key, which can delete the wrong record
when names collide across runs. Update the cleanup logic in test.afterAll to
track the inserted baseball_games id from the create flow in
e2e/baseball-box-score.spec.ts, then query/delete by that id rather than using
.in('opponent_name', createdOpponents). Keep the linked baseball_events cleanup,
but derive event deletion from the captured game record so the teardown only
removes rows created by this test.
In `@scripts/backfill-baseball-slash-lines.ts`:
- Around line 146-161: The per-row update failures in main() are only counted in
errors and logged, but they never affect the script’s exit status. Update the
end-of-run flow in scripts/backfill-baseball-slash-lines.ts so that the main()
summary checks errors after the loop and exits non-zero when any updates failed,
while preserving the existing DRY run behavior. Use the existing errors counter
and the main()/process.exit handling near the bottom of the script to make the
failure signal explicit for CI and ops tooling.
- Around line 81-90: The scan of baseball_player_aggregates in the backfill
script is unbounded and can hit the PostgREST 1000-row cap, causing later rows
to be skipped. Update the backfill flow in
scripts/backfill-baseball-slash-lines.ts to fetch rows in pages with a stable
.order() (or switch to the shared fetchAllRowsResult helper) before entering the
backfill loop, so all AggregateRow records are processed consistently across
reruns.
In `@scripts/e2e-supabase-admin.ts`:
- Around line 21-26: getE2eAdminClient currently reads NEXT_PUBLIC_SUPABASE_URL
and SUPABASE_SERVICE_ROLE_KEY directly from process.env, which skips .env.local
for local runs. Update the env resolution in getE2eAdminClient so it loads
values from .env.local first and falls back to process.env, then keep the
existing null guard and createClient call unchanged.
In `@src/app/baseball/`(dashboard)/_components/breadcrumb-label.tsx:
- Around line 26-55: The BreadcrumbLabelContext value in BreadcrumbLabelProvider
causes all consumers to re-render on any label change because labels is shared
in a single context value. If this pattern will be reused with multiple
simultaneous consumers, split read/write concerns or provide a more selective
subscription so only the affected pathname consumer updates; use
BreadcrumbLabelProvider, BreadcrumbLabelContext, and the setLabel/clearLabel
state updates as the main touchpoints.
In `@src/app/baseball/`(dashboard)/dashboard/calendar/page.tsx:
- Around line 22-26: The page-level defaultEndTime helper is duplicated with the
same logic in useCalendarEvents, so consolidate it into
src/hooks/useCalendarEvents.ts by exporting the shared helper there and update
the calendar page to import and use that exported function instead of defining
its own copy. Keep the existing CalendarEvent import usage aligned with the
shared module so the fallback logic lives in one place and can’t drift.
In `@src/app/baseball/`(dashboard)/dashboard/dev-plan/DevPlanClient.tsx:
- Around line 70-90: getDaysUntil is still parsing date-only strings through new
Date(dateStr), which causes UTC-to-local drift and off-by-one labels for non-UTC
users. Update getDaysUntil in DevPlanClient to parse the YYYY-MM-DD components
directly and build the target date in local time before calculating diffDays,
instead of mutating a UTC-parsed Date with setHours. Keep the existing return
shape and label logic, but make sure the day calculation is based on a locally
constructed date so overdue/due-today status stays accurate.
In `@src/app/baseball/`(dashboard)/dashboard/lift/_lift-athlete-context.ts:
- Around line 33-44: The team lookup in resolvePlayerTeamId is too ambiguous
because it queries baseball_team_members by player_id alone and then relies on
maybeSingle, which can fail when a player has multiple memberships. Update this
helper to explicitly select the active/latest membership (using the existing
team membership fields/ordering in baseball_team_members) before returning
team_id, so the Lift context resolves a single team deterministically instead of
collapsing to null.
In `@src/app/baseball/`(dashboard)/dashboard/lift/[sessionId]/page.tsx:
- Around line 35-79: `fetchSessionWithExercises` is ignoring Supabase `error`
values on the session, exercise, and set-result queries, which can turn real DB
failures into false “missing” or empty results. Update the reads in
`fetchSessionWithExercises` to destructure and check `error` separately from
`data`, log failures, and return/throw appropriately instead of treating them as
empty data. Keep `notFound()` behavior only for the successful no-row case when
the primary session lookup returns null, and make sure the downstream page logic
that calls `fetchSessionWithExercises` still distinguishes errors from a truly
missing session.
In `@src/app/baseball/`(dashboard)/dashboard/lift/page.tsx:
- Around line 79-109: The LiftOnboardingGate usage in the dashboard lift page is
passing through a possibly-null athleteId from getPlayerLiftOnboardingState,
which can cause readLiftOnboardingFlag/writeLiftOnboardingFlag to share state
under a literal null key. Update the render logic around PlayerLiftHomeClient
and LiftOnboardingGate so the gate only mounts when a real athleteId is
available, and otherwise skip the onboarding gate entirely for unresolved
athletes; use the existing getPlayerLiftOnboardingState and LiftOnboardingGate
symbols to keep the fix localized.
- Around line 37-70: The fetchPlayerSessions helper is swallowing Supabase
failures by ignoring the error from both helm_lifting_sessions queries, which
makes RLS denials, timeouts, or bad filters look like genuine empty results.
Update fetchPlayerSessions in dashboard/lift/page.tsx to check the error
returned by each fromUntyped call, log or propagate it with clear context, and
avoid returning empty upcoming/recent arrays when the query actually failed;
keep the successful-path behavior for real no-data cases.
In `@src/app/baseball/`(dashboard)/dashboard/performance/live/page.tsx:
- Around line 104-109: The query in live/page.tsx is filtering
helm_lifting_set_results by session_id even though the available row shape and
current live-session data use session_exercise_id. Update the data flow around
the performance live page to collect the relevant session_exercise ids from
helm_lifting_session_exercises and use those ids in the fromUntyped(...,
'helm_lifting_set_results') query instead of sessionIds. Keep the existing
select/order shape, but make the filter align with HelmLiftingSetResultRow and
the surrounding session_exercises logic so actual_load and rpe populate
correctly.
- Around line 63-69: The live performance query is using UTC-based “today,”
which can shift `scheduled_date`, `checkin_date`, and availability filtering for
non-UTC teams. Update
`src/app/baseball/(dashboard)/dashboard/performance/live/page.tsx` to derive the
date using the team’s timezone via `resolveTeamTimezone()` and `todayIsoInTz()`
from `src/lib/baseball/daily-contract/contract-day.ts` instead of `new
Date().toISOString()`. Apply the team-local date consistently before the
`fromUntyped(...).select('*')` query and any related filters so `today` matches
the team’s calendar day.
In `@src/app/baseball/`(dashboard)/dashboard/readiness/__tests__/page.test.tsx:
- Around line 44-59: The test is re-implementing the Supabase client inline
instead of using the shared fake. Replace the local vi.mock for createClient in
page.test.tsx with createFakeSupabase from src/test/fixtures/fake-supabase, and
seed baseball_teams there so the test uses the shared
.from().select().eq().maybeSingle contract instead of duplicating it.
In `@src/app/baseball/`(dashboard)/dashboard/scouting/page.tsx:
- Around line 1-122: The Scouting page duplicates the same hub-landing grid
pattern already used by the Operations landing, so extract the shared
presentational logic into a reusable helper such as HubLandingGrid in the Living
Annual components area. Move the common card grid,
Link/PaperCard/IconChevronRight rendering, empty state, and nav-context
filtering pattern out of ScoutingPage and let each hub page pass only its own
card list, ink/theme classes, and empty-state copy. Keep the route guard and
page-specific registry/card-id setup in ScoutingPage, and reference the shared
BaseballNavEntry-based card shape so the extracted component stays compatible
with the existing nav registry.
In `@src/app/lifting/actions/__tests__/groups.test.ts`:
- Around line 92-125: Add regression coverage for the member-mutation actions in
the groups test suite: the current `createGroup`, `updateGroup`, and
`deleteGroup` checks do not verify the `addGroupMember` and `removeGroupMember`
revalidation behavior mentioned by the suite header. Extend this test file with
cases that call `addGroupMember` and `removeGroupMember`, then assert
`revalidatePath` includes both the lifting groups path and the baseball
performance groups path, matching the existing pattern used for the other
`groups` actions.
In `@src/components/baseball/calendar/CalendarFairway.tsx`:
- Around line 79-98: The recruiting-empty state in CalendarFairway is missing
the required icon, so update the `recruitingEmpty` branch to satisfy the compact
empty-state pattern. Either pass an icon into `EditorsLetter` alongside the
existing `title`, `body`, and `action`, or, if this pattern is meant to apply
broadly, add an `icon` prop support to the shared `EditorsLetter` component and
use it here. Keep the location tied to `CalendarFairway` and `EditorsLetter` so
the empty state remains “icon + short title + 1-sentence + 1 CTA.”
In `@src/components/lifting/groups/StrengthGroupsClient.tsx`:
- Around line 412-435: The UI in StrengthGroupsClient only blocks
Archive/Add/Remove for dynamic groups, but the server actions still allow direct
mutations. Update the server-side guards in groups.ts, specifically the
archive/add/remove action handlers, to check selected group/group_type before
any writes to helm_lifting_groups or helm_lifting_group_members. Reuse the same
dynamic-group check used by StrengthGroupsClient so rule-managed groups are
rejected even when actions are called directly.
In `@src/lib/baseball/__tests__/resolve-active-hub.test.ts`:
- Around line 212-221: The invariant test in resolve-active-hub.test.ts only
covers four coach hubs, so expand the parameterized cases in the COACH_*_TABS
check to include all hubs defined by COACH_HUB_ORDER from hub-definitions.ts,
including dashboard, team, recruiting, and academics. Update the test data
imports and assertions so every hub’s rendered tab list is verified to stay at 3
or fewer subtabs, not just the four currently listed.
In `@supabase/migrations/20260708010000_bridge_baseball_coach_lifting_edit.sql`:
- Around line 17-19: The SECURITY DEFINER function in this migration uses the
wrong search_path setting and lacks the required inline justification. Update
the function definition to use the empty search path form required by the
migration rules, and add a brief inline comment next to the SECURITY DEFINER
declaration explaining why the elevation is necessary. Use the existing function
body reference and schema-qualified object usage in this migration to keep the
change minimal.
- Around line 14-36: The bridge trigger in
public.bridge_baseball_coach_lifting_access only grants can_edit on
insert/update and never revokes stale access when a coach changes org or is
deleted. Update the trigger logic around trg_bridge_baseball_coach_lifting so it
also handles OLD vs NEW organization_id/user_id on UPDATE to remove or downgrade
the previous helm_lifting_org_viewers grant, and add an AFTER DELETE trigger on
public.baseball_coaches to revoke the corresponding grant for the deleted row.
Be careful to scope revocation to only the bridge-derived permission so existing
independent access is not removed.
---
Outside diff comments:
In `@src/app/baseball/actions/calendar.ts`:
- Around line 240-267: The secondary baseball insert paths in the calendar
action still cast `supabase` to `any` and disable linting, but `fromUntyped()`
is already available in `calendar.ts` and should be used for both the
`baseball_event_attendance` and `baseball_games` writes. Update those insert
calls to use `fromUntyped()` directly, remove the redundant
`eslint-disable-next-line` comments and `any` casts, and keep the existing error
handling and revalidation behavior unchanged.
---
Duplicate comments:
In `@src/app/baseball/`(dashboard)/dashboard/operations/page.tsx:
- Around line 1-117: This page is duplicating the same hub-landing card-grid
pattern seen in the Scouting landing, so extract the shared rendering into the
existing HubLandingGrid-style component or helper and reuse it here. Keep
Operations-specific data only in OPERATIONS_CARD_IDS, the masthead copy, and the
team ink/theme; move the shared requireBaseballCoachRoute,
getBaseballNavContext, getBaseballNavEntry, isBaseballNavEntryVisible, and
cards-to-PaperCard/Link rendering logic into the common abstraction. Use
OperationsPage and the shared grid component to locate the split points cleanly.
🪄 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: 0d024fe4-118e-4768-bcd8-2e51743ccde4
⛔ Files ignored due to path filters (1)
src/lib/types/database.tsis excluded by!src/lib/types/database.ts
📒 Files selected for processing (106)
docs/baseball/COHERENCE_RULING_2026-07-08.mde2e/README.mde2e/baseball-box-score.spec.tsmemory/projects/golfhelm.mdscripts/backfill-baseball-slash-lines.tsscripts/baseballhelm-command-center.mjsscripts/baseballhelm-loc.mjsscripts/e2e-supabase-admin.tsscripts/seed-rini-baseball-demo.tssrc/app/baseball/(coach-dashboard)/coach/layout.tsxsrc/app/baseball/(coach-dashboard)/coach/template.tsxsrc/app/baseball/(coach-dashboard)/error.tsxsrc/app/baseball/(dashboard)/BaseballFairwayShell.tsxsrc/app/baseball/(dashboard)/_components/breadcrumb-label.tsxsrc/app/baseball/(dashboard)/_components/breadcrumbs.tssrc/app/baseball/(dashboard)/_components/hub-definitions.tssrc/app/baseball/(dashboard)/_components/resolve-active-hub.tssrc/app/baseball/(dashboard)/dashboard/academics/AcademicsClient.tsxsrc/app/baseball/(dashboard)/dashboard/announcements/page.tsxsrc/app/baseball/(dashboard)/dashboard/calendar/page.tsxsrc/app/baseball/(dashboard)/dashboard/dev-plan/DevPlanClient.tsxsrc/app/baseball/(dashboard)/dashboard/dev-plan/page.tsxsrc/app/baseball/(dashboard)/dashboard/dev-plans/[id]/page.tsxsrc/app/baseball/(dashboard)/dashboard/documents/documents-client.tsxsrc/app/baseball/(dashboard)/dashboard/documents/page.tsxsrc/app/baseball/(dashboard)/dashboard/events/EventsClient.tsxsrc/app/baseball/(dashboard)/dashboard/lift/[sessionId]/page.tsxsrc/app/baseball/(dashboard)/dashboard/lift/_lift-athlete-context.tssrc/app/baseball/(dashboard)/dashboard/lift/page.tsxsrc/app/baseball/(dashboard)/dashboard/messages/loading.tsxsrc/app/baseball/(dashboard)/dashboard/messages/page.tsxsrc/app/baseball/(dashboard)/dashboard/operations/page.tsxsrc/app/baseball/(dashboard)/dashboard/performance/groups/page.tsxsrc/app/baseball/(dashboard)/dashboard/performance/live/page.tsxsrc/app/baseball/(dashboard)/dashboard/performance/programs/[programId]/page.tsxsrc/app/baseball/(dashboard)/dashboard/performance/programs/page.tsxsrc/app/baseball/(dashboard)/dashboard/players/[id]/page.tsxsrc/app/baseball/(dashboard)/dashboard/players/[id]/profile/error.tsxsrc/app/baseball/(dashboard)/dashboard/players/[id]/profile/page.tsxsrc/app/baseball/(dashboard)/dashboard/players/[id]/stats/page.tsxsrc/app/baseball/(dashboard)/dashboard/practice/page.tsxsrc/app/baseball/(dashboard)/dashboard/readiness/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/readiness/page.tsxsrc/app/baseball/(dashboard)/dashboard/roster/RosterClient.tsxsrc/app/baseball/(dashboard)/dashboard/roster/RosterFairway.tsxsrc/app/baseball/(dashboard)/dashboard/scouting/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/[gameId]/page.tsxsrc/app/baseball/(dashboard)/dashboard/travel/page.tsxsrc/app/baseball/(dashboard)/layout.tsxsrc/app/baseball/(player-dashboard)/player/layout.tsxsrc/app/baseball/actions/__tests__/route-shell-contract.test.tssrc/app/baseball/actions/academics.tssrc/app/baseball/actions/calendar.tssrc/app/baseball/actions/games.tssrc/app/baseball/actions/messages.tssrc/app/lifting/actions/__tests__/groups.test.tssrc/app/lifting/actions/__tests__/player-sessions.test.tssrc/app/lifting/actions/__tests__/programs.test.tssrc/app/lifting/actions/__tests__/sessions.test.tssrc/app/lifting/actions/groups.tssrc/app/lifting/actions/player-sessions.tssrc/app/lifting/actions/programs.tssrc/app/lifting/actions/sessions.tssrc/components/baseball/BaseballShellLayout.tsxsrc/components/baseball/NotificationBell.tsxsrc/components/baseball/ReadModelStateNotice.tsxsrc/components/baseball/calendar/BaseballCalendarWrapper.tsxsrc/components/baseball/calendar/CalendarFairway.tsxsrc/components/baseball/dashboard-shell.tsxsrc/components/baseball/dashboard/dashboard-types.tssrc/components/baseball/documents/DocumentsFairway.tsxsrc/components/baseball/living-annual/RuledStatLine.tsxsrc/components/baseball/performance/LiveWeightRoom.tsxsrc/components/baseball/performance/PlayerLiftHomeClient.tsxsrc/components/baseball/performance/PlayerLiftSessionClient.tsxsrc/components/baseball/performance/PlayerLiftToday.tsxsrc/components/baseball/performance/ProgramEditorClient.tsxsrc/components/baseball/performance/ProgramListClient.tsxsrc/components/baseball/performance/StrengthGroupsClient.tsxsrc/components/baseball/recruiting-philosophy/MatchScoreBadge.tsxsrc/components/baseball/stats-center/StatsCenterClient.tsxsrc/components/baseball/travel/TravelClient.tsxsrc/components/layout/sidebar.tsxsrc/components/layout/team-switcher.tsxsrc/components/lifting/groups/StrengthGroupsClient.tsxsrc/components/lifting/groups/__tests__/StrengthGroupsClient.dynamic.test.tsxsrc/components/messages/EmptyChatState.tsxsrc/contracts/baseball/product-trust.contract.test.tssrc/hooks/__tests__/use-baseball-auth.test.tsxsrc/hooks/use-baseball-auth.tssrc/hooks/useCalendarEvents.tssrc/lib/baseball/__tests__/breadcrumbs.test.tssrc/lib/baseball/__tests__/nav-manifest.test.tssrc/lib/baseball/__tests__/resolve-active-hub.test.tssrc/lib/baseball/nav-manifest.tssrc/lib/baseball/nav-registry.tssrc/lib/baseball/read-models/__tests__/command-center.test.tssrc/lib/baseball/read-models/command-center.tssrc/lib/baseball/read-models/player-snapshot-cards.tssrc/lib/baseball/route-contract.tssrc/lib/baseball/stat-layer-manifest.tssrc/lib/recruiting/match-calculator.tssrc/test/helpers/baseball-route-inventory.tssupabase/migrations/20260708010000_bridge_baseball_coach_lifting_edit.sqlsupabase/migrations/20260708011000_baseball_player_stats_drift_columns.sql
💤 Files with no reviewable changes (22)
- src/components/baseball/performance/LiveWeightRoom.tsx
- src/components/baseball/performance/ProgramListClient.tsx
- src/components/baseball/performance/ProgramEditorClient.tsx
- src/components/baseball/dashboard/dashboard-types.ts
- src/components/baseball/performance/PlayerLiftHomeClient.tsx
- scripts/baseballhelm-loc.mjs
- src/components/baseball/BaseballShellLayout.tsx
- src/app/baseball/actions/tests/route-shell-contract.test.ts
- src/components/baseball/recruiting-philosophy/MatchScoreBadge.tsx
- src/app/baseball/(dashboard)/dashboard/players/[id]/profile/page.tsx
- src/app/baseball/(coach-dashboard)/error.tsx
- src/test/helpers/baseball-route-inventory.ts
- src/components/layout/team-switcher.tsx
- src/components/baseball/performance/PlayerLiftSessionClient.tsx
- src/app/baseball/(coach-dashboard)/coach/layout.tsx
- src/components/baseball/dashboard-shell.tsx
- src/app/baseball/(dashboard)/dashboard/players/[id]/profile/error.tsx
- scripts/baseballhelm-command-center.mjs
- src/lib/recruiting/match-calculator.ts
- src/app/baseball/(coach-dashboard)/coach/template.tsx
- src/components/baseball/performance/StrengthGroupsClient.tsx
- src/components/layout/sidebar.tsx
Comment on lines
+95
to
+119
| test.afterAll(async () => { | ||
| if (createdOpponents.length === 0) return; | ||
| const supabase = getServiceRoleClient(); | ||
| if (!supabase) return; | ||
|
|
||
| // The create form defaults `create_calendar_event` to true, so each | ||
| // created game also has a linked baseball_events row (event_id) — | ||
| // delete both so no orphaned event survives the game's deletion. | ||
| const { data: games } = await supabase | ||
| .from('baseball_games') | ||
| .select('id, event_id') | ||
| .in('opponent_name', createdOpponents); | ||
|
|
||
| const gameIds = (games ?? []).map((g) => g.id); | ||
| const eventIds = (games ?? []) | ||
| .map((g) => g.event_id) | ||
| .filter((id): id is string => Boolean(id)); | ||
|
|
||
| if (gameIds.length > 0) { | ||
| await supabase.from('baseball_games').delete().in('id', gameIds); | ||
| } | ||
| if (eventIds.length > 0) { | ||
| await supabase.from('baseball_events').delete().in('id', eventIds); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the opponent-name generator embeds enough entropy to avoid cross-worker collisions.
rg -n "opponent" e2e/baseball-box-score.spec.ts -B2 -A10 | rg -n "const opponent"Repository: njrini99-code/helmv3
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section with line numbers and surrounding context.
sed -n '130,170p' e2e/baseball-box-score.spec.ts | cat -n
# Check whether the teardown already has any team-scoping data available nearby.
rg -n "team_id|opponent_name|createdOpponents|create_calendar_event|baseball_games|baseball_events" e2e/baseball-box-score.spec.tsRepository: njrini99-code/helmv3
Length of output: 3035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,130p' e2e/baseball-box-score.spec.ts | cat -nRepository: njrini99-code/helmv3
Length of output: 6535
Delete teardown rows by game ID, not opponent_name. e2e/baseball-box-score.spec.ts:103-106 still matches on opponent_name, while e2e/baseball-box-score.spec.ts:141 only adds Date.now() entropy. That isn’t collision-proof across parallel runs, so teardown can delete the wrong game; capture the created game ID and delete by id 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 `@e2e/baseball-box-score.spec.ts` around lines 95 - 119, Teardown is still
identifying rows by opponent_name instead of the created game’s primary key,
which can delete the wrong record when names collide across runs. Update the
cleanup logic in test.afterAll to track the inserted baseball_games id from the
create flow in e2e/baseball-box-score.spec.ts, then query/delete by that id
rather than using .in('opponent_name', createdOpponents). Keep the linked
baseball_events cleanup, but derive event deletion from the captured game record
so the teardown only removes rows created by this test.
Comment on lines
+81
to
+90
| const { data: rows, error: fetchError } = await supabase | ||
| .from('baseball_player_aggregates') | ||
| .select('player_id, team_id, career_avg, career_obp, career_slg, career_ops') | ||
| .returns<AggregateRow[]>(); | ||
|
|
||
| if (fetchError) throw fetchError; | ||
| if (!rows || rows.length === 0) { | ||
| console.log('No baseball_player_aggregates rows found — nothing to backfill.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm actual row count and the project's PostgREST row cap before deciding severity.
echo "Run against the target Supabase project:"
echo "select count(*) from baseball_player_aggregates;"Repository: njrini99-code/helmv3
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting scripts/backfill-baseball-slash-lines.ts and nearby references"
file="scripts/backfill-baseball-slash-lines.ts"
if [ -f "$file" ]; then
echo "== $file (lines 1-220) =="
cat -n "$file" | sed -n '1,220p'
else
echo "Missing: $file"
fi
echo
echo "== References to baseball_player_aggregates =="
rg -n "baseball_player_aggregates|db-max-rows|range\\(|select\\(" scripts src supabase memory .github -g '!**/node_modules/**' || true
echo
echo "== Files that look related to backfills or row-paging =="
fd -a "backfill-baseball-slash-lines.ts|baseball.*backfill|backfill" scripts src supabase memory .github || trueRepository: njrini99-code/helmv3
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="scripts/backfill-baseball-slash-lines.ts"
echo "== $file =="
cat -n "$file" | sed -n '1,220p'
echo
echo "== baseball_player_aggregates usages with context =="
rg -n -A3 -B3 "baseball_player_aggregates" \
scripts src/lib/baseball src/app -g '!**/node_modules/**' || true
echo
echo "== any explicit pagination helpers used for Supabase row fetches =="
rg -n "range\\(|fetch-all-rows|db-max-rows|1000-row cap|paginate past PostgREST" \
scripts src/lib/baseball src/app -g '!**/node_modules/**' || trueRepository: njrini99-code/helmv3
Length of output: 50376
Paginate the baseball_player_aggregates scan (scripts/backfill-baseball-slash-lines.ts:81-90)
select('player_id, team_id, career_avg, career_obp, career_slg, career_ops') is unbounded, so PostgREST will stop at its 1000-row cap. Any aggregates past the first page never reach the backfill loop, and reruns keep missing the same rows. Use a paged query with a stable .order() (or the shared fetchAllRowsResult helper) before iterating.
🤖 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/backfill-baseball-slash-lines.ts` around lines 81 - 90, The scan of
baseball_player_aggregates in the backfill script is unbounded and can hit the
PostgREST 1000-row cap, causing later rows to be skipped. Update the backfill
flow in scripts/backfill-baseball-slash-lines.ts to fetch rows in pages with a
stable .order() (or switch to the shared fetchAllRowsResult helper) before
entering the backfill loop, so all AggregateRow records are processed
consistently across reruns.
Comment on lines
+146
to
+161
| if (!DRY) { | ||
| const { error: updateError } = await supabase | ||
| .from('baseball_player_aggregates') | ||
| .update({ career_obp: obp, career_slg: slg, career_ops: ops }) | ||
| .eq('player_id', row.player_id) | ||
| .eq('team_id', row.team_id); | ||
|
|
||
| if (updateError) { | ||
| console.warn(` ⚠ ${label}: update failed — ${updateError.message}`); | ||
| errors++; | ||
| changed--; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.log(`\n${DRY ? '[DRY RUN] ' : ''}Done. scanned=${scanned} changed=${changed} unchanged=${unchanged} errors=${errors}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Script exits 0 even when per-row updates fail.
errors is tracked (line 97, incremented at 119/155) but never affects the process exit code — only an uncaught exception in main() triggers process.exit(1) (line 179-182). A run with errors > 0 still exits cleanly, so CI/ops tooling invoking this script has no signal that some rows were skipped.
🚦 Fail loudly on partial errors
console.log(`\n${DRY ? '[DRY RUN] ' : ''}Done. scanned=${scanned} changed=${changed} unchanged=${unchanged} errors=${errors}`);
+
+ if (errors > 0) {
+ console.error(`\n${errors} row(s) failed — exiting non-zero.`);
+ process.exitCode = 1;
+ }📝 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 (!DRY) { | |
| const { error: updateError } = await supabase | |
| .from('baseball_player_aggregates') | |
| .update({ career_obp: obp, career_slg: slg, career_ops: ops }) | |
| .eq('player_id', row.player_id) | |
| .eq('team_id', row.team_id); | |
| if (updateError) { | |
| console.warn(` ⚠ ${label}: update failed — ${updateError.message}`); | |
| errors++; | |
| changed--; | |
| } | |
| } | |
| } | |
| console.log(`\n${DRY ? '[DRY RUN] ' : ''}Done. scanned=${scanned} changed=${changed} unchanged=${unchanged} errors=${errors}`); | |
| if (!DRY) { | |
| const { error: updateError } = await supabase | |
| .from('baseball_player_aggregates') | |
| .update({ career_obp: obp, career_slg: slg, career_ops: ops }) | |
| .eq('player_id', row.player_id) | |
| .eq('team_id', row.team_id); | |
| if (updateError) { | |
| console.warn(` ⚠ ${label}: update failed — ${updateError.message}`); | |
| errors++; | |
| changed--; | |
| } | |
| } | |
| } | |
| console.log(`\n${DRY ? '[DRY RUN] ' : ''}Done. scanned=${scanned} changed=${changed} unchanged=${unchanged} errors=${errors}`); | |
| if (errors > 0) { | |
| console.error(`\n${errors} row(s) failed — exiting non-zero.`); | |
| process.exitCode = 1; | |
| } |
🤖 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/backfill-baseball-slash-lines.ts` around lines 146 - 161, The per-row
update failures in main() are only counted in errors and logged, but they never
affect the script’s exit status. Update the end-of-run flow in
scripts/backfill-baseball-slash-lines.ts so that the main() summary checks
errors after the loop and exits non-zero when any updates failed, while
preserving the existing DRY run behavior. Use the existing errors counter and
the main()/process.exit handling near the bottom of the script to make the
failure signal explicit for CI and ops tooling.
Comment on lines
+21
to
+26
| export function getE2eAdminClient() { | ||
| const url = (process.env.NEXT_PUBLIC_SUPABASE_URL ?? '').trim(); | ||
| const key = (process.env.SUPABASE_SERVICE_ROLE_KEY ?? '').trim(); | ||
| if (!url || !key) return null; | ||
| return createClient(url, key, { auth: { persistSession: false, autoRefreshToken: false } }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getE2eAdminClient reads process.env directly, bypassing .env.local.
Path instructions for scripts/**/*.{ts,mjs,js,sh} require: "Node scripts must read env from .env.local first, then process.env." Lines 22-23 go straight to process.env.NEXT_PUBLIC_SUPABASE_URL / process.env.SUPABASE_SERVICE_ROLE_KEY. Fine in CI (env exported directly per the file's own doc comment), but breaks local .env.local-only setups for anyone running these specs outside CI.
🔧 Proposed fix
+import { config } from 'dotenv';
+config({ path: '.env.local' });
+
export function getE2eAdminClient() {
const url = (process.env.NEXT_PUBLIC_SUPABASE_URL ?? '').trim();
const key = (process.env.SUPABASE_SERVICE_ROLE_KEY ?? '').trim();📝 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 getE2eAdminClient() { | |
| const url = (process.env.NEXT_PUBLIC_SUPABASE_URL ?? '').trim(); | |
| const key = (process.env.SUPABASE_SERVICE_ROLE_KEY ?? '').trim(); | |
| if (!url || !key) return null; | |
| return createClient(url, key, { auth: { persistSession: false, autoRefreshToken: false } }); | |
| } | |
| import { config } from 'dotenv'; | |
| config({ path: '.env.local' }); | |
| export function getE2eAdminClient() { | |
| const url = (process.env.NEXT_PUBLIC_SUPABASE_URL ?? '').trim(); | |
| const key = (process.env.SUPABASE_SERVICE_ROLE_KEY ?? '').trim(); | |
| if (!url || !key) return null; | |
| return createClient(url, key, { auth: { persistSession: false, autoRefreshToken: false } }); | |
| } |
🤖 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/e2e-supabase-admin.ts` around lines 21 - 26, getE2eAdminClient
currently reads NEXT_PUBLIC_SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY directly
from process.env, which skips .env.local for local runs. Update the env
resolution in getE2eAdminClient so it loads values from .env.local first and
falls back to process.env, then keep the existing null guard and createClient
call unchanged.
Source: Path instructions
Comment on lines
+26
to
+55
| interface BreadcrumbLabelContextValue { | ||
| labels: Readonly<Record<string, string>>; | ||
| setLabel: (pathname: string, name: string) => void; | ||
| clearLabel: (pathname: string) => void; | ||
| } | ||
|
|
||
| const BreadcrumbLabelContext = createContext<BreadcrumbLabelContextValue | null>(null); | ||
|
|
||
| /** Mounted once by BaseballFairwayShell, above both the shell chrome (reader) | ||
| * and `{children}` (writer) so both sides share one context instance. */ | ||
| export function BreadcrumbLabelProvider({ children }: { children: React.ReactNode }) { | ||
| const [labels, setLabels] = useState<Readonly<Record<string, string>>>({}); | ||
|
|
||
| const setLabel = useCallback((pathname: string, name: string) => { | ||
| setLabels((prev) => (prev[pathname] === name ? prev : { ...prev, [pathname]: name })); | ||
| }, []); | ||
|
|
||
| const clearLabel = useCallback((pathname: string) => { | ||
| setLabels((prev) => { | ||
| if (!(pathname in prev)) return prev; | ||
| const next = { ...prev }; | ||
| delete next[pathname]; | ||
| return next; | ||
| }); | ||
| }, []); | ||
|
|
||
| const value = useMemo(() => ({ labels, setLabel, clearLabel }), [labels, setLabel, clearLabel]); | ||
|
|
||
| return <BreadcrumbLabelContext.Provider value={value}>{children}</BreadcrumbLabelContext.Provider>; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Context value re-renders all consumers on every label mutation.
value gets a new identity whenever any pathname's label changes (shared labels state), so useContext re-renders both the shell chrome reader and the currently-mounted page writer even when their own label didn't change. Harmless today since only one dynamic detail page is mounted at a time, but worth knowing if this pattern gets reused for a surface with multiple simultaneous consumers (e.g. parallel routes).
🤖 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)/_components/breadcrumb-label.tsx around lines
26 - 55, The BreadcrumbLabelContext value in BreadcrumbLabelProvider causes all
consumers to re-render on any label change because labels is shared in a single
context value. If this pattern will be reused with multiple simultaneous
consumers, split read/write concerns or provide a more selective subscription so
only the affected pathname consumer updates; use BreadcrumbLabelProvider,
BreadcrumbLabelContext, and the setLabel/clearLabel state updates as the main
touchpoints.
Comment on lines
+79
to
98
| <div className="flex-shrink-0 px-4 pt-4 md:px-6 md:pt-6"> | ||
| <SectionMasthead eyebrow="THE WAR ROOM · CALENDAR" title="Calendar" ink="pursuit" /> | ||
| </div> | ||
| <div className="flex flex-1 items-center justify-center p-6"> | ||
| <EmptyState | ||
| icon={CalendarIcon} | ||
| {/* Not the `calendar` EmptyIssue preset ("No dates on the card") — | ||
| this is the recruiting-specific narrative (camp/official-visit | ||
| windows), so it stays a bespoke EditorsLetter like the other | ||
| Batch A surfaces' non-preset states (e.g. TasksFairway/ | ||
| AnnouncementsFairway "no team selected"). */} | ||
| <EditorsLetter | ||
| ink="pursuit" | ||
| title="Your recruiting calendar is empty" | ||
| description="Camp visits and official visit windows will appear here as you schedule recruiting activity." | ||
| body="Camp visits and official visit windows will appear here as you schedule recruiting activity." | ||
| action={ | ||
| <Button asChild variant="primary"> | ||
| <Link href="/baseball/dashboard/discover">Browse prospects</Link> | ||
| </Button> | ||
| } | ||
| className="max-w-md" | ||
| /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Recruiting-empty state has no icon — violates the "icon + short title + 1-sentence + 1 CTA" empty-state rule.
EditorsLetter (used here for the recruitingEmpty branch) has no icon prop at all — only title/body/signoff/action/live. This replaced an EmptyState that (per the AI summary) previously rendered a CalendarIcon. As per coding guidelines, src/components/**/*.{ts,tsx} requires: "Empty states must be compact: icon + short title + 1-sentence + 1 CTA." As currently written, this empty state has title + sentence + CTA but no icon.
If this bespoke editorial pattern (mirroring other Batch A "no team selected" states) is an intentional design-system exception for narrative empty states, that's fine — but it's worth confirming with the design-system doc rather than silently dropping the icon requirement here.
💡 One way to keep the icon without touching the shared EditorsLetter component
<EditorsLetter
ink="pursuit"
title="Your recruiting calendar is empty"
body="Camp visits and official visit windows will appear here as you schedule recruiting activity."
+ signoff="— add a camp or visit window from Discover to get started"
action={
<Button asChild variant="primary">
<Link href="/baseball/dashboard/discover">Browse prospects</Link>
</Button>
}
className="max-w-md"
/>Alternatively, add an icon prop to the shared EditorsLetter component if this pattern is meant to satisfy the compact-empty-state rule everywhere it's used.
🤖 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/calendar/CalendarFairway.tsx` around lines 79 - 98,
The recruiting-empty state in CalendarFairway is missing the required icon, so
update the `recruitingEmpty` branch to satisfy the compact empty-state pattern.
Either pass an icon into `EditorsLetter` alongside the existing `title`, `body`,
and `action`, or, if this pattern is meant to apply broadly, add an `icon` prop
support to the shared `EditorsLetter` component and use it here. Keep the
location tied to `CalendarFairway` and `EditorsLetter` so the empty state
remains “icon + short title + 1-sentence + 1 CTA.”
Source: Path instructions
Comment on lines
412
to
435
| {canEdit && ( | ||
| <Button | ||
| size="sm" | ||
| variant="ghost" | ||
| className="text-warm-400 hover:text-red-600" | ||
| disabled={isPending} | ||
| onClick={() => handleArchive(selectedGroup.id)} | ||
| > | ||
| <IconTrash size={14} className="mr-1" /> Archive | ||
| </Button> | ||
| isSelectedDynamic ? ( | ||
| <Tooltip content={DYNAMIC_GROUP_TOOLTIP}> | ||
| <Button | ||
| size="sm" | ||
| variant="ghost" | ||
| className="text-warm-300" | ||
| disabled | ||
| > | ||
| <IconTrash size={14} className="mr-1" /> Archive | ||
| </Button> | ||
| </Tooltip> | ||
| ) : ( | ||
| <Button | ||
| size="sm" | ||
| variant="ghost" | ||
| className="text-warm-400 hover:text-red-600" | ||
| disabled={isPending} | ||
| onClick={() => handleArchive(selectedGroup.id)} | ||
| > | ||
| <IconTrash size={14} className="mr-1" /> Archive | ||
| </Button> | ||
| ) | ||
| )} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "export async function (deleteGroup|addGroupMember|removeGroupMember)" -A 20 src/app/lifting/actions/groups.ts
rg -n "group_type" src/app/lifting/actions/groups.tsRepository: njrini99-code/helmv3
Length of output: 612
🏁 Script executed:
#!/bin/bash
sed -n '1,280p' src/app/lifting/actions/groups.tsRepository: njrini99-code/helmv3
Length of output: 10456
🏁 Script executed:
#!/bin/bash
sed -n '280,520p' src/app/lifting/actions/groups.tsRepository: njrini99-code/helmv3
Length of output: 2248
🏁 Script executed:
#!/bin/bash
nl -ba src/app/lifting/actions/groups.ts | sed -n '120,320p'Repository: njrini99-code/helmv3
Length of output: 194
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path('src/app/lifting/actions/groups.ts')
for i, line in enumerate(path.read_text().splitlines(), 1):
if 120 <= i <= 320:
print(f"{i}:{line}")
PYRepository: njrini99-code/helmv3
Length of output: 8131
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path('src/app/lifting/actions/groups.ts')
for i, line in enumerate(path.read_text().splitlines(), 1):
if 315 <= i <= 380:
print(f"{i}:{line}")
PYRepository: njrini99-code/helmv3
Length of output: 1161
Block dynamic-group writes in the server actions.
src/components/lifting/groups/StrengthGroupsClient.tsx:412-507 only disables Archive/Add/Remove in the UI. src/app/lifting/actions/groups.ts:213-243, 250-308, and 315-340 still mutate helm_lifting_groups / helm_lifting_group_members without checking group_type !== 'dynamic', so any direct action call can still edit a rule-managed group. Add the same guard server-side before those writes.
🤖 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/lifting/groups/StrengthGroupsClient.tsx` around lines 412 -
435, The UI in StrengthGroupsClient only blocks Archive/Add/Remove for dynamic
groups, but the server actions still allow direct mutations. Update the
server-side guards in groups.ts, specifically the archive/add/remove action
handlers, to check selected group/group_type before any writes to
helm_lifting_groups or helm_lifting_group_members. Reuse the same dynamic-group
check used by StrengthGroupsClient so rule-managed groups are rejected even when
actions are called directly.
Comment on lines
+212
to
221
| describe('every coach hub caps at 3 rendered subtabs (Ruling 2)', () => { | ||
| it.each([ | ||
| ['stats-performance', COACH_STATS_TABS], | ||
| ['development', COACH_DEVELOPMENT_TABS], | ||
| ['management', COACH_MANAGEMENT_TABS], | ||
| ['messages', COACH_MESSAGES_TABS], | ||
| ] as const)('%s renders ≤3 tabs', (_hub, tabs) => { | ||
| expect(tabs.length).toBeLessThanOrEqual(3); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
"Caps at 3 subtabs" invariant only checked for 4 of 8 coach hubs.
COACH_HUB_ORDER (hub-definitions.ts) has 8 hubs: dashboard, messages, team, stats-performance, development, recruiting, academics, management. This parameterized test only asserts the cap for stats-performance, development, management, messages — the four hubs the ruling touched. team, recruiting, academics, and dashboard aren't verified here, so a future regression that pushes any of those past 3 tabs won't be caught by this specific guardrail (even though the PR's stated goal is "no hub exceeding three subtabs" system-wide).
✅ Proposed fix — cover all 8 hubs
describe('every coach hub caps at 3 rendered subtabs (Ruling 2)', () => {
it.each([
+ ['dashboard', COACH_DASHBOARD_TABS],
+ ['team', COACH_TEAM_TABS],
['stats-performance', COACH_STATS_TABS],
['development', COACH_DEVELOPMENT_TABS],
+ ['recruiting', COACH_RECRUITING_TABS],
+ ['academics', COACH_ACADEMICS_TABS],
['management', COACH_MANAGEMENT_TABS],
['messages', COACH_MESSAGES_TABS],
] as const)('%s renders ≤3 tabs', (_hub, tabs) => {
expect(tabs.length).toBeLessThanOrEqual(3);
});
});(Add the corresponding imports from hub-definitions.ts.)
🤖 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__/resolve-active-hub.test.ts` around lines 212 -
221, The invariant test in resolve-active-hub.test.ts only covers four coach
hubs, so expand the parameterized cases in the COACH_*_TABS check to include all
hubs defined by COACH_HUB_ORDER from hub-definitions.ts, including dashboard,
team, recruiting, and academics. Update the test data imports and assertions so
every hub’s rendered tab list is verified to stay at 3 or fewer subtabs, not
just the four currently listed.
Comment on lines
+14
to
+36
| CREATE OR REPLACE FUNCTION public.bridge_baseball_coach_lifting_access() | ||
| RETURNS trigger | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path TO 'public', 'pg_temp' | ||
| AS $$ | ||
| BEGIN | ||
| IF NEW.organization_id IS NOT NULL AND NEW.user_id IS NOT NULL THEN | ||
| INSERT INTO public.helm_lifting_org_viewers (organization_id, user_id, sport, can_edit, granted_by) | ||
| VALUES (NEW.organization_id, NEW.user_id, 'baseball', true, 'trigger:baseball-coach-lifting-bridge') | ||
| ON CONFLICT (organization_id, user_id, sport) | ||
| DO UPDATE SET can_edit = true; | ||
| END IF; | ||
| RETURN NEW; | ||
| END; | ||
| $$; | ||
|
|
||
| REVOKE EXECUTE ON FUNCTION public.bridge_baseball_coach_lifting_access() FROM PUBLIC, anon, authenticated; | ||
|
|
||
| DROP TRIGGER IF EXISTS trg_bridge_baseball_coach_lifting ON public.baseball_coaches; | ||
| CREATE TRIGGER trg_bridge_baseball_coach_lifting | ||
| AFTER INSERT OR UPDATE OF organization_id, user_id ON public.baseball_coaches | ||
| FOR EACH ROW EXECUTE FUNCTION public.bridge_baseball_coach_lifting_access(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Stale can_edit grants aren't revoked on org change or coach removal.
The trigger only handles INSERT/UPDATE OF organization_id, user_id, and on update it adds a new helm_lifting_org_viewers row for the new org — it never downgrades or removes the row for the coach's previous organization_id. Two concrete leak paths:
- Coach transfers from Org A → Org B: the update fires, granting
can_edit=trueon Org B, but the Org A row (can_edit=true) is left untouched — the coach retains lifting edit access on a team they no longer coach. - Coach row is deleted from
baseball_coachesentirely: noAFTER DELETEtrigger exists, so thehelm_lifting_org_viewersgrant is permanent regardless of employment status.
Since this bridges into an RLS-gating table (helm_lifting_can_edit_org per the migration's own header comment), this is a live authorization surface, not just a data-hygiene nit. Consider either (a) adding an AFTER UPDATE OF organization_id branch that flips can_edit=false (or deletes the row) for OLD.organization_id/OLD.user_id when it differs from NEW, and (b) an AFTER DELETE trigger that revokes the grant for the deleted coach's org/user pair — being careful not to revoke access for a user who is also a legitimate lifting coach/org-viewer independent of this bridge.
🤖 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/20260708010000_bridge_baseball_coach_lifting_edit.sql`
around lines 14 - 36, The bridge trigger in
public.bridge_baseball_coach_lifting_access only grants can_edit on
insert/update and never revokes stale access when a coach changes org or is
deleted. Update the trigger logic around trg_bridge_baseball_coach_lifting so it
also handles OLD vs NEW organization_id/user_id on UPDATE to remove or downgrade
the previous helm_lifting_org_viewers grant, and add an AFTER DELETE trigger on
public.baseball_coaches to revoke the corresponding grant for the deleted row.
Be careful to scope revocation to only the bridge-derived permission so existing
independent access is not removed.
Comment on lines
+17
to
+19
| SECURITY DEFINER | ||
| SET search_path TO 'public', 'pg_temp' | ||
| AS $$ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
SET search_path doesn't match the required empty-string form.
This SECURITY DEFINER function sets SET search_path TO 'public', 'pg_temp' rather than SET search_path = ''. As per path instructions, supabase/migrations/*.sql rule 8 states: "Forbid SECURITY DEFINER functions without an explicit SET search_path = '' and an inline comment justifying the elevation." All object references in the function body are already schema-qualified (public.helm_lifting_org_viewers), which mitigates most of the practical risk, but it's a literal deviation from the stated rule, and there's no inline comment specifically justifying the SECURITY DEFINER elevation itself (the header comment explains the migration's purpose, not the elevation).
Note this also conflicts with docs/v3-rls-template.md's documented convention of SET search_path TO 'public' for existing helpers like is_golf_team_primary_coach — worth reconciling which convention is canonical so future migrations don't alternate between the two.
🤖 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/20260708010000_bridge_baseball_coach_lifting_edit.sql`
around lines 17 - 19, The SECURITY DEFINER function in this migration uses the
wrong search_path setting and lacks the required inline justification. Update
the function definition to use the empty search path form required by the
migration rules, and add a brief inline comment next to the SECURITY DEFINER
declaration explaining why the elevation is necessary. Use the existing function
body reference and schema-qualified object usage in this migration to keep the
change minimal.
Source: Path instructions
This was referenced Jul 10, 2026
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
1 participant
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.
What this is
The overnight consolidation that ends the two-workflow era. Ruling doc:
docs/baseball/COHERENCE_RULING_2026-07-08.md. 24 focused commits (deliberately decomposed per the 2026-06-30 shell postmortem's contrast pattern — each commit is one surface).The four structural fixes
BaseballFairwayShellrenders unconditionally; everyisRedesignEnabled()fork in baseball is gone. Deleted:(coach-dashboard)route group (zero pages),BaseballShellLayout,dashboard-shell.tsx,layout/sidebar.tsx+team-switcher.tsx(zero importers — golf rendersGolfSidebar.tsx, never these). The env-flag regression path to the 31-item flat sidebar no longer exists.nav-manifest.test.tsfor every coach type + players. Stats' 13-tab row → Stats Center · Games · Postgame (Season/Upload/Import fold inside). Management's 13-tab row → Decision Room · Settings · Organization (Settings = card-grid landing, splice deleted). NewOperationsandScoutinglandings inherit registry gating. Announcements lives in Messages. Players hitting/dashboard/practiceland on their own practice surface.src/components/lifting/*; six legacy duplicates deleted; zerobaseball_lift_*writes remain in baseball surfaces. Coach edit access preserved viahelm_lifting_org_viewersbridge (migration20260708010000, trigger-maintained; 9/10 coaches verified — 10th has no org). Lifting mutations now revalidate both portals. Dynamic groups (both prod rows) render first-class read-only.Prod operations already executed (independent of this deploy)
E2E Created Opponent%junk games + 160 linked events (86% of the games table)career_obp/slg/ops(20/22 rows) via new idempotent script20260708010000,20260708011000)Gates (all green)
tsc --noEmit0 · eslint 0 warnings · lint-ratchet 0 · unit 4501 passed (445 files; +39 new tests) ·next build0 · sweeps: 0 legacy nav arrays, 0 baseball_lift_ writes in baseball surfaces, remaining shell refs comment-only.Known follow-ups (documented in ruling doc)
E2E still targets prod Supabase (cleanup added; isolated project needs owner). Dynamic-group rule authoring not rebuilt (read-only + nothing recomputes memberships). Travel card redesign deferred (PR #555 collision).
(player-dashboard)→(dashboard)fold deferred (URLs load-bearing).🤖 Generated with Claude Code
https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu