Skip to content

BaseballHelm coherence: one shell, capped nav IA, one Lift Lab, data honesty #792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
njrini99-code merged 19 commits into from
Jul 9, 2026
Merged

BaseballHelm coherence: one shell, capped nav IA, one Lift Lab, data honesty #792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
211fee2
docs(baseball): coherence ruling for overnight consolidation — one sh…
Jul 8, 2026
4ed9617
refactor(baseball): render BaseballFairwayShell unconditionally, dele…
Jul 8, 2026
cf0d024
refactor(baseball): delete legacy BaseballShellLayout/dashboard-shell…
Jul 8, 2026
660f51f
refactor(baseball): remove remaining isRedesignEnabled forks in calen…
Jul 8, 2026
44b036d
refactor(baseball): navigation IA restructure — cap every hub at ≤3 s…
Jul 8, 2026
01adf35
feat(baseball): add Operations + Scouting hub-landing pages (Ruling 2)
Jul 8, 2026
31ea5e8
refactor(baseball): remove duplicate <Header> mount from Settings pag…
Jul 8, 2026
672f970
test(baseball): lock the ≤3-subtabs-per-hub anti-regression contract …
Jul 8, 2026
255ea47
fix(baseball): team-local 'today' for readiness + command center; rea…
Jul 8, 2026
356e44c
fix(baseball): stats honesty — tri-state eligibility, drop phantom EX…
Jul 8, 2026
7df21b6
fix(baseball): breadcrumbs never render raw UUIDs; detail pages suppl…
Jul 8, 2026
6ba82a4
fix(baseball): calendar/event integrity + E2E self-cleanup
Jul 8, 2026
bb386d5
refactor(baseball): one Lift Lab — baseball routes now use the canoni…
Jul 8, 2026
0e3a0b5
chore(db): baseball-coach lifting-edit bridge + player-stats drift co…
Jul 8, 2026
ecb435b
chore(baseball): delete dead code + duplicate surfaces; fix dev-plan …
Jul 8, 2026
d357fc6
style(baseball): Living Annual polish — duplicate headers gone, no am…
Jul 8, 2026
0eb85ca
fix(baseball): team-local week bounds, snapshot stat fallbacks, cross…
Jul 8, 2026
cd68952
test(baseball): align contracts with tonight's consolidation + regen …
Jul 8, 2026
063382f
fix(e2e): move service-role teardown client into scripts/ helper (ast…
Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions docs/baseball/COHERENCE_RULING_2026-07-08.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
# BaseballHelm Coherence Ruling — 2026-07-08 (overnight consolidation)

**Authority:** Commander decision doc for the one-night coherence mission. Supersedes conflicting
guidance in CANONICAL_SPEC §2.1 (stale 10-item nav). Builds on COACH_NAV_8TAB_PROPOSAL.md
(owner-approved 2026-07-01) and the 2026-06-30 shell postmortem's unexecuted recommendations.
Baseline: `origin/main` e63de6044 — tsc/lint/ratchet/unit all green.

## Ruling 1 — ONE shell: BaseballFairwayShell, unconditional

`NEXT_PUBLIC_REDESIGN=true` is what prod serves. The legacy fork is pure regression risk
(`.env.example` defaults it off). Therefore:

- `(dashboard)/layout.tsx` and `(player-dashboard)/player/layout.tsx` render
`BaseballFairwayShell` **unconditionally** — delete the `isRedesignEnabled()` forks
(baseball layouts only; golf untouched).
- Delete `src/app/baseball/(coach-dashboard)/` entirely (zero page.tsx, confirmed dead).
- Remove the **baseball** nav paths from `src/components/layout/sidebar.tsx`
(5 legacy arrays: collegeTeamNav/hsCoachTeamNav/jucoTeamNav/showcaseOrgNav/playerTeamNav +
`buildCondensedBaseballNavigation`); the golf branch stays byte-identical.
- Delete `BaseballShellLayout`/`BaseballDashboardShell` baseball render path once unreferenced.
- Route groups `(dashboard)` + `(player-dashboard)` both stay (URLs are load-bearing:
PWA start_url, bookmarks). Folding them is deferred — not tonight's risk.

## Ruling 2 — Navigation IA: ≤8 primary, ≤3 subtabs per primary (hard caps)

Top level keeps the owner-approved hub set (College sees 7, HS 6, JUCO 8 — all ≤8):
**Dashboard · Team · Messages · Stats & Performance · Development · Recruiting ·
Academics (JUCO) · Management**. The fix is inside the hubs — every destination stays
reachable ≤2 clicks (subtab landing pages surface deeper routes as cards/CTAs; command
palette stays flat with everything). Deep routes keep their URLs; `resolve-active-hub`
maps them to the owning subtab for highlight + breadcrumbs.

| Hub | Subtabs (≤3) | Folded in (reachable from landing) |
|---|---|---|
| Dashboard | Overview · Signals | — |
| Team | Roster · Calendar · Operations | Operations = new landing: Documents, Travel, Practice Planner, Practice Effectiveness |
| Messages | Messages · Announcements | announcements moves here from Team (it's comms) |
| Stats & Performance | Stats Center · Games · Postgame | Season = view inside Stats Center; Upload + Import Center = CTAs inside Stats Center |
| Development | Dev Plans · Training · Videos | Training = existing /dashboard/performance landing → Programs, Live Weight Room, Builder, Groups |
| Recruiting | Pipeline · Discover · Scouting | Scouting = new landing: Watchlist, Compare, Saved Comparisons, Scout Packets, Camps |
| Academics | (single page) | JUCO only |
| Management | Decision Room · Settings · Organization | Settings = existing card-grid landing (KEEP grid, DELETE the 9-tab splice from COACH_MANAGEMENT_TABS); Organization = org/teams/events (Showcase types) |

Player nav (Fairway): Today · Schedule · My Stats · Development · Team · Messages ·
My Profile (+ Recruiting when activated) — already ≤8; enforce ≤3 subtabs per hub the same way.
Players hitting `/baseball/dashboard/practice` redirect to `/baseball/player/practice` (canonical
player practice surface).

Nav-manifest test extended: every coach/both registry entry maps to exactly one hub, and no
hub resolves >3 subtabs for any coach type. That test is the anti-regression lock.

## Ruling 3 — ONE Lift Lab

Canonical = `src/components/lifting/*` + `helm_lifting_*`. Repoint the 6 baseball
performance routes at the canonical components; delete `src/components/baseball/performance/*`
(23-file legacy tree, GolfHelm-palette, writes legacy `baseball_lift_*`). Completes the
in-flight unification train.

## Ruling 4 — Data honesty & correctness cluster

- "Today" is **team-local** everywhere (`resolveTeamTimezone` + `todayIsoInTz`), never server-UTC:
readiness page, command-center read-model, player-today read-model.
- Calendar: null `end_time` renders start + 1h default (never zero-duration); events query gets
lookback bound + limit; badge labels pluralize.
- Academics eligibility is tri-state: `null` = gray "Not on file"; red "Ineligible" only for real `false`.
- Roster: drop EXIT V column (column doesn't exist in schema, no write path — honest UI);
backfill career_obp/slg/ops for existing rows (prod data op).
- Breadcrumbs: UUID-shaped segments never title-cased; dynamic routes supply real names
(players/[id], stats/games/[id], dev-plans/[id]).
- `createBaseballEvent` game-insert errors checked, not swallowed.
- E2E: spec cleans up its own rows (service-role delete in teardown); prod junk rows
(`E2E Created Opponent%`) deleted as a data op; isolated E2E project documented as follow-up
needing owner (new Supabase project).
- Seed script gets realistic event times (practice 15:30–17:30, games 13:00–16:00, meetings 12:00–13:00 team-local);
demo team's polluted event rows corrected in place.

## Ruling 5 — Dead code deleted, not layered over

`(coach-dashboard)`; legacy sidebar baseball arrays; legacy shell baseball path; legacy Lift Lab
tree; `players/[id]/profile` duplicate page (canonical = `players/[id]` PlayerProfileClient);
knip-confirmed orphans (MatchScoreBadge, match-calculator, dashboard-types); 5 orphaned
`baseballhelm-*.{mjs,workflow.js}` scripts (superseded by Helm Bridge).

## Out of scope tonight (documented, not forgotten)

Route-group merge of `(player-dashboard)` into `(dashboard)`; dedicated E2E Supabase project;
PlayerPassportCard→Fairway preview swap if polish wave runs out of clock; full 3-lane
Living-Annual masthead vision (ui-migration-map L56) — the 8-hub IA is the stepping stone.
37 changes: 26 additions & 11 deletions e2e/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,17 +188,32 @@ present (it isn't, on fork PRs, matching the existing `E2E_GOLF_*` secret
pattern in that workflow). When the secret is absent, the seed step is
skipped entirely and the three specs above self-skip rather than fail.

### Known limitation: "create game" / "create camp" specs are non-destructive only on the create side

`GameCard`'s delete affordance is intentionally hidden in the UI (a
permanent `hidden` class), so the box-score spec's "create a new game" test
cannot clean up the game row it creates via the UI — each CI run adds one
new `baseball_games` row with a unique opponent name
(`E2E Created Opponent ${Date.now()}`) to the test database. This is a
known, accepted trade-off (documented here rather than worked around) since
the created row doesn't affect any other spec's assertions. The Camps
spec's create/delete round-trip *is* fully self-cleaning, since the Camps
UI does expose a working delete action.
### Cleanup: "create game" spec self-cleans via a service-role teardown (not the UI)

`GameCard`'s delete affordance is still intentionally hidden in the UI (a
permanent `hidden` class), so `baseball-box-score.spec.ts`'s "should create
a new game and redirect to its box-score entry page" test cannot clean up
the `baseball_games` row it creates through the app UI. It creates a real
row (plus a linked `baseball_events` row, since the create form defaults
`create_calendar_event` to `true`) tagged with a unique opponent name
(`E2E Created Opponent ${Date.now()}`).

Rather than accept that as permanent test-database pollution, the
`Coach - Create New Game` describe block now has a `test.afterAll` teardown
that deletes exactly the `baseball_games` row(s) (and their linked
`baseball_events` row(s)) it created, by opponent name, via a service-role
Supabase client — the same construction pattern
`scripts/seed-baseball-e2e.ts` uses for seeding
(`NEXT_PUBLIC_SUPABASE_URL` + `SUPABASE_SERVICE_ROLE_KEY`, session-less).
The teardown is a silent no-op (never a failure) when the service-role key
isn't present in the environment, matching every other seed/cleanup path in
this suite.

This test had no cleanup prior to 2026-07-08 and had accumulated ~160 junk
`baseball_games` rows in the shared database, purged via a one-off SQL
cleanup that same night. The Camps spec's create/delete round-trip was
already fully self-cleaning via its working UI delete action and is
unaffected by this change.

## CI/CD Integration

Expand Down
56 changes: 52 additions & 4 deletions e2e/baseball-box-score.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { test, expect, type Page } from '@playwright/test';
import { getE2eAdminClient } from '../scripts/e2e-supabase-admin';
import { loginAsCoach, loginAsPlayer } from './helpers/auth';
import { waitForPageLoad } from './helpers/common';

Expand All @@ -23,10 +24,13 @@ import { waitForPageLoad } from './helpers/common';
* including `status`), per the "destructive tests rely on seed reset"
* pattern used throughout this fixture.
*
* The "create game" test adds one extra (non-seeded) game row each run —
* there is currently no reachable delete affordance in the UI (GamesList's
* delete button is shipped with a permanent `hidden` class), so this is a
* known, documented trade-off rather than an oversight. See e2e/README.md.
* The "create game" test adds one extra (non-seeded) game row each run.
* `GamesList`'s delete button is still shipped with a permanent `hidden`
* class, so there is no reachable UI delete affordance — cleanup instead
* happens directly against the database: a `test.afterAll` below deletes
* every `baseball_games` row (+ its linked `baseball_events` row) this file
* created, via the same service-role client construction
* `scripts/seed-baseball-e2e.ts` uses for seeding. See e2e/README.md.
*
* Gated on PLAYWRIGHT_BASEBALL_SEEDED=1, with a per-test self-skip if the
* login fixture is unavailable in the target environment — same pattern as
Expand All @@ -42,6 +46,18 @@ const SEEDED =
const SCHEDULED_OPPONENT = 'Riverside University';
const COMPLETED_OPPONENT = 'Eastview College';

/**
* Service-role Supabase client for teardown-only writes (deleting rows this
* spec itself created) — provided by scripts/e2e-supabase-admin.ts so this
* spec never references the service-role env var directly (ast-grep rule
* helmv3-no-service-role-key). Returns `null` (teardown becomes a no-op)
* rather than throwing when the env is missing — cleanup must never fail an
* otherwise-passing run.
*/
function getServiceRoleClient() {
return getE2eAdminClient();
}

async function loginCoachOrSkip(page: Page) {
try {
await loginAsCoach(page);
Expand All @@ -65,12 +81,43 @@ async function loginPlayerOrSkip(page: Page) {
test.describe('Coach - Create New Game', () => {
test.skip(!SEEDED, 'no seeded baseball team fixture (set PLAYWRIGHT_BASEBALL_SEEDED=1)');

// Opponent names created by the "create new game" test below — tracked so
// the afterAll teardown can delete exactly (and only) the rows this file
// created, never touching the seeded fixture games.
const createdOpponents: string[] = [];

test.beforeEach(async ({ page }) => {
await loginCoachOrSkip(page);
await page.goto('/baseball/dashboard/stats/games/create');
await waitForPageLoad(page);
});

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);
}
Comment on lines +103 to +118

@qodo-code-review qodo-code-review Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

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

});
Comment on lines +95 to +119

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ 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.ts

Repository: 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 -n

Repository: 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.


test('should display the new game form with date, opponent, and venue fields', async ({ page }) => {
await expect(page.getByRole('heading', { name: /Add Game/i })).toBeVisible();
await expect(page.locator('#new-game-date')).toBeVisible();
Expand All @@ -92,6 +139,7 @@ test.describe('Coach - Create New Game', () => {

test('should create a new game and redirect to its box-score entry page', async ({ page }) => {
const opponent = `E2E Created Opponent ${Date.now()}`;
createdOpponents.push(opponent);
const futureDate = new Date();
futureDate.setDate(futureDate.getDate() + 45);
const dateStr = futureDate.toISOString().slice(0, 10);
Expand Down
17 changes: 5 additions & 12 deletions memory/projects/golfhelm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ src/lib/coachhelm/
<!-- AUTOGEN:routes:start -->
<!-- DO NOT EDIT — regenerated by scripts/regen-docs.mjs -->

**231 routes** (source: `src/app/**/page.tsx`).
**224 routes** (source: `src/app/**/page.tsx`).

<details><summary>Full alphabetical route list</summary>

Expand All @@ -273,6 +273,7 @@ src/lib/coachhelm/
- `/admin/activity`
- `/admin/auth`
- `/admin/baseball`
- `/admin/ben-leah`
- `/admin/deploys`
- `/admin/errors`
- `/admin/errors/[fingerprint]`
Expand All @@ -284,14 +285,11 @@ src/lib/coachhelm/
- `/admin/users`
- `/admin/users/[id]`
- `/admin/users/[id]/view-as`
- `/admin/work`
- `/baseball`
- `/baseball/admin/demo-sessions`
- `/baseball/coach`
- `/baseball/coach-onboarding`
- `/baseball/coach/college`
- `/baseball/coach/high-school`
- `/baseball/coach/juco`
- `/baseball/coach/showcase`
- `/baseball/complete-signup`
- `/baseball/dashboard`
- `/baseball/dashboard/academics`
Expand Down Expand Up @@ -320,6 +318,7 @@ src/lib/coachhelm/
- `/baseball/dashboard/messages`
- `/baseball/dashboard/messages/[id]`
- `/baseball/dashboard/my-stats`
- `/baseball/dashboard/operations`
- `/baseball/dashboard/organization`
- `/baseball/dashboard/performance`
- `/baseball/dashboard/performance/builder`
Expand All @@ -331,7 +330,6 @@ src/lib/coachhelm/
- `/baseball/dashboard/pipeline`
- `/baseball/dashboard/players/[id]`
- `/baseball/dashboard/players/[id]/passport`
- `/baseball/dashboard/players/[id]/profile`
- `/baseball/dashboard/players/[id]/scout-packet`
- `/baseball/dashboard/players/[id]/scout-packet/preview`
- `/baseball/dashboard/players/[id]/stats`
Expand All @@ -343,6 +341,7 @@ src/lib/coachhelm/
- `/baseball/dashboard/readiness`
- `/baseball/dashboard/roster`
- `/baseball/dashboard/scout-packets`
- `/baseball/dashboard/scouting`
- `/baseball/dashboard/settings`
- `/baseball/dashboard/settings/ai`
- `/baseball/dashboard/settings/appearance`
Expand Down Expand Up @@ -370,12 +369,10 @@ src/lib/coachhelm/
- `/baseball/dashboard/stats/games`
- `/baseball/dashboard/stats/games/[gameId]`
- `/baseball/dashboard/stats/games/create`
- `/baseball/dashboard/stats/games/new`
- `/baseball/dashboard/stats/season`
- `/baseball/dashboard/stats/upload`
- `/baseball/dashboard/tasks`
- `/baseball/dashboard/team`
- `/baseball/dashboard/team/high-school`
- `/baseball/dashboard/teams`
- `/baseball/dashboard/travel`
- `/baseball/dashboard/videos`
Expand All @@ -389,12 +386,8 @@ src/lib/coachhelm/
- `/baseball/packet/[token]`
- `/baseball/player`
- `/baseball/player/[id]`
- `/baseball/player/college`
- `/baseball/player/high-school`
- `/baseball/player/juco`
- `/baseball/player/passport`
- `/baseball/player/practice`
- `/baseball/player/showcase`
- `/baseball/player/timeline`
- `/baseball/player/today`
- `/baseball/program/[id]`
Expand Down
Loading
Loading