docs(qa): BaseballHelm Fairway visual audit pack (coach + player)#788
Conversation
Coach and player screenshot packs with per-route manifests linking page files, database tables, actions/read-models, and audit notes. Includes build script to regenerate from Playwright audit output.
|
Too many files changed for review. ( Bypass the limit by tagging |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Summary by CodeRabbit
WalkthroughAdds a baseball QA audit pack generator and generated coach/player audit docs, updates recruiting navigation for ChangesBaseball QA Visual Audit Pack
Baseball navigation and data flow updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ 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-stats-smoke.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. scripts/seed-rini-baseball-demo.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. scripts/verify-baseball-demo-coverage.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * Usage: node scripts/build-baseball-qa-pack.mjs | ||
| */ | ||
| import { readFileSync, writeFileSync, mkdirSync, copyFileSync, existsSync } from 'fs'; | ||
| import { join, basename } from 'path'; |
| */ | ||
| import { readFileSync, writeFileSync, mkdirSync, copyFileSync, existsSync } from 'fs'; | ||
| import { join, basename } from 'path'; | ||
| import { globSync } from 'glob'; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/baseball/(dashboard)/_components/hub-definitions.ts (1)
442-445: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the player redirect before the dashboard shell renders
PLAYER_RECRUITING_TABSno longer owns/baseball/dashboard/college-interest(src/app/baseball/(dashboard)/_components/hub-definitions.ts:441-445vs.src/app/baseball/(dashboard)/_components/resolve-active-hub.ts:94-119), andCollegeInterestClient.tsx:191-260only sends non-coach players away inuseEffect. That leaves a deep-link window where the dashboard shell can mount with no active player hub highlighted. Push the player redirect to the page/layout boundary or make this route bypass the shell for player visitors.🤖 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/hub-definitions.ts around lines 442 - 445, The player redirect for the college-interest flow is happening too late, allowing the dashboard shell to render before the user is sent away. Move the non-coach player redirect out of CollegeInterestClient’s useEffect and into the page/layout boundary so it runs before the shell mounts, or make the /baseball/dashboard/college-interest route bypass the dashboard shell entirely for player visitors. Update the routing logic around CollegeInterestClient, resolve-active-hub, and the dashboard shell entry point so deep links never show an unhighlighted hub.src/app/baseball/(dashboard)/dashboard/college-interest/CollegeInterestClient.tsx (1)
41-63: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtract
EngagementEventto@/lib/typessrc/app/baseball/(dashboard)/dashboard/college-interest/CollegeInterestClient.tsx:41-63duplicates the same shape insrc/hooks/use-analytics.ts:27-44; share one type so the query result stays in sync across both files.🤖 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/college-interest/CollegeInterestClient.tsx around lines 41 - 63, The EngagementEvent shape is duplicated in CollegeInterestClient and use-analytics, so it should be shared from a single source of truth. Move the EngagementEvent type into `@/lib/types`, then import and use it in CollegeInterestClient and the analytics hook so both query result typings stay aligned. Update any local type references in the affected components/hooks to use the shared type instead of redefining it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/use-analytics.ts`:
- Around line 225-226: Confirm that the `useAnalytics` effect continues to
depend only on `user` and that the `supabase` client is truly stable by
construction from the `useRef`-based setup used in this hook. If the client is
not guaranteed stable, move its initialization so the ref is created once and
reused across renders; otherwise keep the current `[user]` dependency array with
the eslint-disable in place.
- Line 117: The handled failure in use-analytics.ts is currently falling back to
the empty state without recording eventsError anywhere. Update the useAnalytics
flow around the eventsError || !events check to send the query failure through
src/lib/error-logging.ts logError, or capture it with Sentry captureException,
before returning the empty analytics state. Keep the fallback behavior, but
ensure the error is logged with enough context to distinguish a real
Postgres/RLS failure from a genuine no-data result.
- Line 3: The analytics hook is still creating a browser Supabase client during
render, which causes a new instance on every re-render and can leave extra auth
state behind. Update use-analytics to avoid calling createClient() inside useRef
initialization or any render-time path; instead create the client lazily once
inside the hook using a stable initialization pattern and keep the existing
instance across renders. Use the createClient function from
src/lib/supabase/client.ts and the useAnalytics hook as the main locations to
adjust.
In `@src/hooks/use-messages.ts`:
- Around line 282-285: The participant name lookup in use-messages is relying on
parallel participant_ids and participant_names arrays whose order is not
guaranteed, so fix the conversation data shape to be deterministic. Update the
baseline migration query that builds the conversation payload so participant_ids
and participant_names come from the same ordered source (or better, a single
ordered participant record array), and avoid dropping entries when a users row
is missing. Then adjust the lookup logic in use-messages to read the name from
that deterministic structure using otherUserId rather than assuming array
positions always align.
In `@src/lib/baseball/nav-registry.ts`:
- Line 125: Add a regression test in resolve-active-hub.test.ts that directly
exercises resolveActiveHub for the new college-interest nav entry: call
resolveActiveHub with pathname '/baseball/dashboard/college-interest' and role
'coach', and assert it resolves to hub 'recruiting' with the 'college-interest'
tab id present. Use the existing resolveActiveHub test setup/helpers so the new
assertion covers the nav-registry entry introduced in nav-registry.ts.
---
Outside diff comments:
In `@src/app/baseball/`(dashboard)/_components/hub-definitions.ts:
- Around line 442-445: The player redirect for the college-interest flow is
happening too late, allowing the dashboard shell to render before the user is
sent away. Move the non-coach player redirect out of CollegeInterestClient’s
useEffect and into the page/layout boundary so it runs before the shell mounts,
or make the /baseball/dashboard/college-interest route bypass the dashboard
shell entirely for player visitors. Update the routing logic around
CollegeInterestClient, resolve-active-hub, and the dashboard shell entry point
so deep links never show an unhighlighted hub.
In
`@src/app/baseball/`(dashboard)/dashboard/college-interest/CollegeInterestClient.tsx:
- Around line 41-63: The EngagementEvent shape is duplicated in
CollegeInterestClient and use-analytics, so it should be shared from a single
source of truth. Move the EngagementEvent type into `@/lib/types`, then import and
use it in CollegeInterestClient and the analytics hook so both query result
typings stay aligned. Update any local type references in the affected
components/hooks to use the shared type instead of redefining it.
🪄 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: a3a669c8-8557-4dee-b337-d68948bd66d7
📒 Files selected for processing (12)
docs/qa/baseball-fairway-visual-audit-2026-07-04/NAV_ISSUES.mddocs/qa/baseball-fairway-visual-audit-2026-07-04/README.mdscripts/__tests__/baseball-stale-route-links.test.mjsscripts/build-baseball-qa-pack.mjssrc/app/baseball/(dashboard)/_components/hub-definitions.tssrc/app/baseball/(dashboard)/dashboard/college-interest/CollegeInterestClient.tsxsrc/components/messages/ConversationList.tsxsrc/hooks/use-analytics.tssrc/hooks/use-messages.tssrc/lib/baseball/__tests__/resolve-active-hub.test.tssrc/lib/baseball/nav-registry.tssrc/lib/types/messages.ts
| 'use client'; | ||
|
|
||
| import { useState, useEffect } from 'react'; | ||
| import { useState, useEffect, useRef } from 'react'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n "createClient|createBrowserClient|useRef\\(createClient\\(|useState\\(\\(\\) => createClient\\(" src/lib/supabase src/hooks/use-analytics.tsRepository: njrini99-code/helmv3
Length of output: 765
🌐 Web query:
Supabase GoTrueClient _startAutoRefresh autoRefreshToken default createBrowserClient per-instance setInterval visibilitychange listener
💡 Result:
In the Supabase SDK, the GoTrueClient handles token refreshing via an internal _startAutoRefreshToken process, which manages session longevity [1][2]. Key implementation details regarding your query include: Auto-Refresh Mechanism The client uses a timer to schedule token refreshes [1][3]. When a session is saved or recovered, the client calculates the time remaining until expiration and calls _startAutoRefreshToken to schedule a refresh task [1]. The _startAutoRefreshToken method uses setTimeout to trigger _callRefreshToken shortly before the token expires [1][3]. If the refresh fails due to network issues, the client implements an exponential backoff strategy for retries [1]. Visibility Change Listener The GoTrueClient includes a _handleVisibilityChange method that listens for the browser's visibilitychange event [4]. When a tab or window returns to the foreground (document.visibilityState === 'visible'), the client triggers a session recovery and refresh process (_recoverAndRefresh) to ensure the session is still valid [5][4]. This is particularly important for recovering from tab suspension [5]. Calling the public startAutoRefresh or stopAutoRefresh methods manually will remove this managed visibility change callback, shifting the responsibility for managing visibility to the developer [2][6]. CreateBrowserClient Integration The createBrowserClient function from the @supabase/ssr package is designed to simplify client initialization in browser environments [7][8]. It automatically configures autoRefreshToken to true by default when running in a browser [7][9]. Historically, earlier versions of createBrowserClient would override user-provided autoRefreshToken settings, but recent updates (e.g., @supabase/ssr@0.10.1 and later) have been patched to respect developer-provided configuration options using nullish coalescing [10][9]. Per-Instance Behavior GoTrueClient manages its own internal state and timers on a per-instance basis [1][11]. Each SupabaseClient (and its nested auth client) maintains its own refresh timer, storage key references, and visibility listeners [11][4]. This ensures that multiple client instances do not interfere with each other's refresh cycles, though developers should be mindful of creating multiple singleton-style browser clients, which the @supabase/ssr library manages via a cached singleton pattern [7].
Citations:
- 1: https://github.com/supabase/gotrue-js/blob/8d7eef85a41c5e94a1336f0f44eacc5253186e9b/src/GoTrueClient.ts
- 2: https://supabase.github.io/auth-js/v2/classes/GoTrueClient.html
- 3: https://github.com/supabase/gotrue-js/blob/f5d9800c42b3be62496aa0daece8c13625ce85ac/src/GoTrueClient.ts
- 4: https://github.com/supabase/gotrue-js/blob/30fb5729a1988dbd7f61d58ec10eb81de63f93c7/src/GoTrueClient.ts
- 5: fix(auth,supabase): recover from client corruption after tab suspension supabase/supabase-js#2228
- 6: https://supabase.com/docs/reference/javascript/auth-startautorefresh
- 7: https://github.com/supabase/ssr/blob/main/src/createBrowserClient.ts
- 8: https://github.com/supabase/auth-helpers/blob/main/packages/ssr/src/createBrowserClient.ts
- 9: supabase/ssr@5f04837
- 10: Bug:
createBrowserClientoverrides user-provideddetectSessionInUrlandflowType, causing silent session destruction supabase/ssr#175 - 11: https://github.com/supabase/auth-js/blob/25c6b42/src/GoTrueClient.ts
Avoid useRef(createClient()) here; it evaluates on every render
src/hooks/use-analytics.ts:60-61 still calls createClient() during render, so each re-render builds and discards a browser Supabase client. src/lib/supabase/client.ts:8-17 wraps createBrowserClient(), which creates per-instance auth refresh state; that pattern can leave extra timers/listeners behind.
🔧 Proposed fix
- const supabaseRef = useRef(createClient());
- const supabase = supabaseRef.current;
+ const [supabase] = useState(() => createClient());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/use-analytics.ts` at line 3, The analytics hook is still creating a
browser Supabase client during render, which causes a new instance on every
re-render and can leave extra auth state behind. Update use-analytics to avoid
calling createClient() inside useRef initialization or any render-time path;
instead create the client lazily once inside the hook using a stable
initialization pattern and keep the existing instance across renders. Use the
createClient function from src/lib/supabase/client.ts and the useAnalytics hook
as the main locations to adjust.
| .order('created_at', { ascending: false }); | ||
|
|
||
| if (!events) { | ||
| if (eventsError || !events) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
eventsError is swallowed silently — no logging before falling back to the empty state.
Per docs/operations/observability-convention.md, client-side handled errors should still reach src/lib/error-logging.ts logError (or Sentry captureException) so a persistent query failure is visible somewhere other than "user sees zero analytics." Right now a real Postgres/RLS error is indistinguishable from "no rows yet."
🔧 Proposed fix
if (eventsError || !events) {
+ if (eventsError) {
+ logError(eventsError, { context: 'useAnalytics.fetchAnalytics' });
+ }
setData({📝 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.
| if (eventsError || !events) { | |
| if (eventsError || !events) { | |
| if (eventsError) { | |
| logError(eventsError, { context: 'useAnalytics.fetchAnalytics' }); | |
| } | |
| setData({ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/use-analytics.ts` at line 117, The handled failure in
use-analytics.ts is currently falling back to the empty state without recording
eventsError anywhere. Update the useAnalytics flow around the eventsError ||
!events check to send the query failure through src/lib/error-logging.ts
logError, or capture it with Sentry captureException, before returning the empty
analytics state. Keep the fallback behavior, but ensure the error is logged with
enough context to distinguish a real Postgres/RLS failure from a genuine no-data
result.
| // eslint-disable-next-line react-hooks/exhaustive-deps -- `supabase` is stable across renders (useRef above). | ||
| }, [user]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Dependency-array fix is correct, contingent on the useRef client being genuinely stable.
Narrowing to [user] with the eslint-disable is the right call given supabase doesn't change identity across renders — the ref's .current binding is indeed stable, so the effect won't loop. This is a real fix for the effect previously depending on a non-memoized supabase. See the comment on lines 60-61 for the underlying construction pattern that still needs cleanup, but the dependency-array reasoning itself is sound.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/use-analytics.ts` around lines 225 - 226, Confirm that the
`useAnalytics` effect continues to depend only on `user` and that the `supabase`
client is truly stable by construction from the `useRef`-based setup used in
this hook. If the client is not guaranteed stable, move its initialization so
the ref is created once and reused across renders; otherwise keep the current
`[user]` dependency array with the eslint-disable in place.
| const otherUserId = conv.participant_ids?.find((id: string) => id !== user.id); | ||
| const otherUserIndex = otherUserId ? conv.participant_ids?.indexOf(otherUserId) ?? -1 : -1; | ||
| const otherUserDisplayName = | ||
| otherUserIndex >= 0 ? conv.participant_names?.[otherUserIndex] ?? null : null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the RPC definition and check how participant_ids / participant_names are aggregated
rg -n "get_baseball_conversations_with_details" -g '*.sql' -A 40Repository: njrini99-code/helmv3
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate RPC definition across repo ==\n'
rg -n "get_baseball_conversations_with_details" .
printf '\n== locate relevant hook lines ==\n'
sed -n '250,330p' src/hooks/use-messages.tsRepository: njrini99-code/helmv3
Length of output: 6497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== baseline function body ==\n'
sed -n '2311,2355p' supabase/migrations/20260527000000_prod_public_baseline.sql
printf '\n== archive function body ==\n'
sed -n '360,430p' supabase/migrations_archive/pre_20260527/042_sport_specific_messaging_tables.sql
printf '\n== fixed/lint migration body ==\n'
sed -n '1,80p' supabase/migrations_archive/pre_20260527/20260518124505_fix_live_db_lint_errors.sqlRepository: njrini99-code/helmv3
Length of output: 8065
Make the participant-name lookup deterministic
src/hooks/use-messages.ts:282-285 indexes participant_names using the position of otherUserId, but supabase/migrations/20260527000000_prod_public_baseline.sql:2311-2333 builds participant_ids and participant_names from separate ARRAY(SELECT ...) subqueries with no shared ORDER BY. That can attach the wrong name to the wrong participant; the JOIN users in the names subquery can also drop a participant entirely if the user row is missing. Build both arrays from the same ordered source, or return a single ordered participant record array 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 `@src/hooks/use-messages.ts` around lines 282 - 285, The participant name
lookup in use-messages is relying on parallel participant_ids and
participant_names arrays whose order is not guaranteed, so fix the conversation
data shape to be deterministic. Update the baseline migration query that builds
the conversation payload so participant_ids and participant_names come from the
same ordered source (or better, a single ordered participant record array), and
avoid dropping entries when a users row is missing. Then adjust the lookup logic
in use-messages to read the name from that deterministic structure using
otherUserId rather than assuming array positions always align.
| | 'analytics' | ||
| | 'announcements' | ||
| | 'pipeline' | ||
| | 'college-interest' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
New college-interest entry looks correct — add a resolveActiveHub regression test.
The entry itself (id, href, role: 'coach', hub: 'recruiting', no capability gate) is consistent with the objective ("Coaches own the actual data surface"). No security/logic issue here.
Given resolve-active-hub.test.ts only exercises /baseball/dashboard/college-interest indirectly (and now via a swapped pathname — see that file's review), add an explicit coach-role assertion that resolveActiveHub({ pathname: '/baseball/dashboard/college-interest', role: 'coach', ... }) resolves to hub 'recruiting' with tab id 'college-interest' present. This is the one nav id this PR actually introduces, and it currently has zero direct hub-resolution coverage.
Also applies to: 563-575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/baseball/nav-registry.ts` at line 125, Add a regression test in
resolve-active-hub.test.ts that directly exercises resolveActiveHub for the new
college-interest nav entry: call resolveActiveHub with pathname
'/baseball/dashboard/college-interest' and role 'coach', and assert it resolves
to hub 'recruiting' with the 'college-interest' tab id present. Use the existing
resolveActiveHub test setup/helpers so the new assertion covers the nav-registry
entry introduced in nav-registry.ts.
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/sidebar.tsx`:
- Around line 216-221: The comment on the branch that returns playerTeamNav is
stale and contradicts the current Recruiting nav entry added via
PLAYER_RECRUITING_TABS and COACH_HUB_DEFS.recruiting.icon. Update or remove the
outdated “ARCHIVED: recruiting nav disabled” comment so it accurately reflects
that recruiting is still part of the team nav, and keep the wording aligned with
the playerTeamNav construction in sidebar.tsx.
- Around line 120-125: The sidebar fallback for the recruiting hub is hardcoding
the label instead of using the mode-specific noun. Update the fallback in
sidebar.tsx where COACH_RECRUITING_HUB is defined, and any other hardcoded
Recruiting label in this flow, to use the same label source already used later
via getBaseballTerminology(ctx).exposureNoun. Keep the hub logic tied to
COACH_RECRUITING_HUB/COACH_RECRUITING_TABS, but make the displayed name resolve
from the terminology helper so it matches the program type before navContext
finishes loading.
In `@src/lib/baseball/__tests__/resolve-active-hub.test.ts`:
- Around line 93-103: Add a negative regression test in resolveActiveHub for the
academics hub using a non-eligible program type such as high_school with
can_view_academics: true, and assert it does not activate academics (hub is
null/undefined or another hub is chosen). Use the existing resolveActiveHub
helper and the academics hub/tab expectations in resolve-active-hub.test.ts to
mirror the positive case while verifying the fallback behavior for ineligible
programs.
🪄 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: 93820902-7320-49d3-953e-428ed96b23ac
📒 Files selected for processing (11)
docs/qa/baseball-fairway-visual-audit-2026-07-04/NAV_ISSUES.mde2e/baseball-stats-smoke.spec.tsscripts/seed-rini-baseball-demo.tsscripts/verify-baseball-demo-coverage.tssrc/app/baseball/(dashboard)/BaseballFairwayShell.tsxsrc/app/baseball/(dashboard)/_components/hub-definitions.tssrc/app/baseball/(dashboard)/_components/resolve-active-hub.tssrc/app/baseball/actions/__tests__/route-shell-contract.test.tssrc/components/layout/sidebar.tsxsrc/lib/baseball/__tests__/nav-manifest.test.tssrc/lib/baseball/__tests__/resolve-active-hub.test.ts
| const COACH_RECRUITING_HUB: SidebarHubItem = { | ||
| name: 'Recruiting', | ||
| href: COACH_RECRUITING_TABS[0]!.href, | ||
| icon: COACH_HUB_DEFS.recruiting.icon, | ||
| hubPrefixes: hubPrefixesFrom(COACH_RECRUITING_TABS), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "exposureNoun" src/lib/baseball/nav-registry.ts -A 3 -B 3Repository: njrini99-code/helmv3
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sidebar file around the referenced lines and search for the nav label helpers.
sed -n '100,180p' src/components/layout/sidebar.tsx
printf '\n---\n'
sed -n '200,240p' src/components/layout/sidebar.tsx
printf '\n---\n'
sed -n '280,340p' src/components/layout/sidebar.tsx
printf '\n---\n'
rg -n "getBaseballTerminology|exposureNoun|Recruiting" src -A 2 -B 2Repository: njrini99-code/helmv3
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect sidebar file around the referenced lines and search for the nav label helpers.
sed -n '100,180p' src/components/layout/sidebar.tsx
printf '\n---\n'
sed -n '200,240p' src/components/layout/sidebar.tsx
printf '\n---\n'
sed -n '280,340p' src/components/layout/sidebar.tsx
printf '\n---\n'
rg -n "getBaseballTerminology|exposureNoun|Recruiting" src -A 2 -B 2Repository: njrini99-code/helmv3
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '100,180p' src/components/layout/sidebar.tsx
printf '\n---\n'
sed -n '200,240p' src/components/layout/sidebar.tsx
printf '\n---\n'
sed -n '280,340p' src/components/layout/sidebar.tsx
printf '\n---\n'
rg -n "getBaseballTerminology|exposureNoun|Recruiting" src -A 2 -B 2Repository: njrini99-code/helmv3
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "function getBaseballTerminology|getBaseballTerminology\(" src memory -A 6 -B 3Repository: njrini99-code/helmv3
Length of output: 6695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'sidebar.tsx excerpt:\n'
sed -n '110,240p' src/components/layout/sidebar.tsx
printf '\n---\nnav-related matches:\n'
rg -n "getBaseballTerminology|exposureNoun|Recruiting" src memory -A 2 -B 2Repository: njrini99-code/helmv3
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the terminology definitions and the tests that exercise them.
sed -n '1,220p' src/lib/baseball/__tests__/program-type-nav-variants.test.ts
printf '\n---\n'
sed -n '1,260p' src/lib/baseball/nav-registry.ts | sed -n '900,1160p'Repository: njrini99-code/helmv3
Length of output: 11009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the specific terminology mappings and the assertions about exposureNoun.
rg -n "exposureNoun|terminology" src/lib/baseball/nav-registry.ts -A 6 -B 6
printf '\n---\n'
sed -n '130,170p' src/lib/baseball/__tests__/program-type-nav-variants.test.tsRepository: njrini99-code/helmv3
Length of output: 2595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the actual exposureNoun definitions/usages without dumping the whole file.
rg -n "exposureNoun" src/lib/baseball src/app/baseball -A 3 -B 3Repository: njrini99-code/helmv3
Length of output: 4998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the variant definitions around any exposure-related terminology fields.
rg -n "exposure" src/lib/baseball/nav-registry.ts -A 5 -B 5Repository: njrini99-code/helmv3
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the terminology object fields that include exposureNoun.
rg -n "exposureNoun\s*:" src/lib/baseball/nav-registry.ts -A 3 -B 3
printf '\n---\n'
rg -n "exposureNoun" src/lib/baseball/__tests__/program-type-nav-variants.test.ts -A 3 -B 3Repository: njrini99-code/helmv3
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the program variant objects that define terminology, if any.
rg -n "exposureNoun|eventNoun|programNoun|rosterNoun" src/lib/baseball/nav-registry.ts -A 4 -B 4Repository: njrini99-code/helmv3
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the variant table entries that set terminology.
rg -n "terminology:" src/lib/baseball/nav-registry.ts -A 10 -B 4Repository: njrini99-code/helmv3
Length of output: 158
Use the mode-specific recruiting noun in the sidebar fallback.
src/components/layout/sidebar.tsx:121 and :217 hardcode Recruiting, but :296 and :328 already pull the label from getBaseballTerminology(ctx).exposureNoun. That noun varies by program type (College Interest, Exposure, Transfer Exposure), so the fallback can flash the wrong label until navContext resolves. Make the fallback share the same label source instead of hardcoding it.
🤖 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/layout/sidebar.tsx` around lines 120 - 125, The sidebar
fallback for the recruiting hub is hardcoding the label instead of using the
mode-specific noun. Update the fallback in sidebar.tsx where
COACH_RECRUITING_HUB is defined, and any other hardcoded Recruiting label in
this flow, to use the same label source already used later via
getBaseballTerminology(ctx).exposureNoun. Keep the hub logic tied to
COACH_RECRUITING_HUB/COACH_RECRUITING_TABS, but make the displayed name resolve
from the terminology helper so it matches the program type before navContext
finishes loading.
| { | ||
| name: 'Recruiting', | ||
| href: PLAYER_RECRUITING_TABS[0]!.href, | ||
| icon: COACH_HUB_DEFS.recruiting.icon, | ||
| hubPrefixes: hubPrefixesFrom(PLAYER_RECRUITING_TABS), | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale "ARCHIVED: recruiting nav disabled" comment now contradicts the code it annotates.
Lines 216-221 add a live "Recruiting" entry to playerTeamNav, but the branch that returns playerTeamNav (shown further down in this file) still carries the comment // ARCHIVED: All players now see team nav only — recruiting nav disabled. That comment is now factually wrong and will mislead the next person touching this branch into thinking recruiting nav is dead code.
📝 Suggested comment fix
} else if (user?.role === 'player') {
- // ARCHIVED: All players now see team nav only — recruiting nav disabled
+ // Player fallback nav (pre-navContext). Mirrors buildCondensedBaseballNavigation's
+ // player branch, including the Recruiting entry.
return playerTeamNav;
}🤖 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/layout/sidebar.tsx` around lines 216 - 221, The comment on the
branch that returns playerTeamNav is stale and contradicts the current
Recruiting nav entry added via PLAYER_RECRUITING_TABS and
COACH_HUB_DEFS.recruiting.icon. Update or remove the outdated “ARCHIVED:
recruiting nav disabled” comment so it accurately reflects that recruiting is
still part of the team nav, and keep the wording aligned with the playerTeamNav
construction in sidebar.tsx.
|
|
||
| it('activates academics for college coaches with the academics capability', () => { | ||
| const hub = resolveActiveHub({ | ||
| pathname: '/baseball/dashboard/academics', | ||
| role: 'coach', | ||
| programType: 'college', | ||
| capabilities: { can_view_academics: true }, | ||
| }); | ||
| expect(hub?.id).toBe('academics'); | ||
| expect(hub?.tabs.map((t) => t.id)).toEqual(['academics']); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a negative test for academics + ineligible program type.
This test proves academics activates for programType: 'college' + can_view_academics, but there's no companion test asserting it stays hidden for a non-eligible program type (e.g. high_school) even when can_view_academics: true is set. Given the removed code path (Line 239-246 in BaseballFairwayShell.tsx, Line 78-91 in resolve-active-hub.ts) used to hard-block this at the hub level, and enforcement now lives purely in tab config, this is exactly the regression this suite should guard against.
✅ Suggested additional test
it('activates academics for college coaches with the academics capability', () => {
const hub = resolveActiveHub({
pathname: '/baseball/dashboard/academics',
role: 'coach',
programType: 'college',
capabilities: { can_view_academics: true },
});
expect(hub?.id).toBe('academics');
expect(hub?.tabs.map((t) => t.id)).toEqual(['academics']);
});
+
+ it('hides academics for ineligible program types even with the capability set', () => {
+ const hub = resolveActiveHub({
+ pathname: '/baseball/dashboard/academics',
+ role: 'coach',
+ programType: 'high_school',
+ capabilities: { can_view_academics: true },
+ });
+ expect(hub?.id).not.toBe('academics');
+ });📝 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.
| it('activates academics for college coaches with the academics capability', () => { | |
| const hub = resolveActiveHub({ | |
| pathname: '/baseball/dashboard/academics', | |
| role: 'coach', | |
| programType: 'college', | |
| capabilities: { can_view_academics: true }, | |
| }); | |
| expect(hub?.id).toBe('academics'); | |
| expect(hub?.tabs.map((t) => t.id)).toEqual(['academics']); | |
| }); | |
| it('activates academics for college coaches with the academics capability', () => { | |
| const hub = resolveActiveHub({ | |
| pathname: '/baseball/dashboard/academics', | |
| role: 'coach', | |
| programType: 'college', | |
| capabilities: { can_view_academics: true }, | |
| }); | |
| expect(hub?.id).toBe('academics'); | |
| expect(hub?.tabs.map((t) => t.id)).toEqual(['academics']); | |
| }); | |
| it('hides academics for ineligible program types even with the capability set', () => { | |
| const hub = resolveActiveHub({ | |
| pathname: '/baseball/dashboard/academics', | |
| role: 'coach', | |
| programType: 'high_school', | |
| capabilities: { can_view_academics: true }, | |
| }); | |
| expect(hub?.id).not.toBe('academics'); | |
| }); |
🤖 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 93 - 103,
Add a negative regression test in resolveActiveHub for the academics hub using a
non-eligible program type such as high_school with can_view_academics: true, and
assert it does not activate academics (hub is null/undefined or another hub is
chosen). Use the existing resolveActiveHub helper and the academics hub/tab
expectations in resolve-active-hub.test.ts to mirror the positive case while
verifying the fallback behavior for ineligible programs.
2 participants
Summary
Adds a labeled visual QA artifact pack from the 2026-07-04 BaseballHelm Fairway click-through audit (coach + player demo accounts).
Contents
docs/qa/baseball-fairway-visual-audit-2026-07-04/coach/INDEX.mdplayer/INDEX.mdEach route entry includes:
{##}-{role}-{route-slug}.png)src/app/baseball/.../page.tsx)ERRORS.md)Regenerate
Other changes
.gitignore: allowdocs/qa/**/*.png(global*.pngignore otherwise blocks screenshots)scripts/build-baseball-qa-pack.mjs: pack builder from audit outputTest plan
coach/INDEX.mdandplayer/INDEX.mdlink to all desktop/mobile shotsmanifest.jsonper role for programmatic lookup