fix(baseball): Fairway routing + Helm Bridge observability (unified)#787
Merged
Conversation
Finish Codex's interrupted Fairway migration: hub-based nav with activeMatch prefixes, player layout shell mounting, Management settings leaves, messages thread ownership in route-contract, legacy bookmark redirects in middleware, and player Settings in the secondary rail. Removes dead redirect stub pages in favor of middleware aliases. Route coverage gaps reduced from 51 to 20 (all intentional auth/public/onboarding surfaces).
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThis PR removes legacy Baseball redirect pages, refactors Baseball auth and shell selection, adds admin work-log and incident-feed surfaces, and introduces Ben-Leah issue tracking plus workflow controls. ChangesBaseball navigation, shells, and legacy routes
Baseball auth, session, and action observability
Admin work log, incident feed, and Ben-Leah tracking
Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 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.0)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 🔧 ESLint
src/lib/server-error-logger.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Comment |
coderabbitai
Bot
added
ci
Combines fix/helm-bridge-observability-baseball-auth (soft-failure logging, incident feed, baseball auth polish, Ben+Leah/work tabs) with the Fairway shell routing and route-contract work. Dashboard layout keeps authVerified on both shell branches after the session guard settles.
cursor
Bot
changed the title
- Derive Fairway bottom tabs from the same hub sections as desktop rail - Fix prefer-const in incident errors query builder - Resolve ESLint token warnings in Ben+Leah workflow select and settings hub
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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/(dashboard)/_components/hub-definitions.ts (1)
78-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded id-based
matchPrefixesassignment is fragile.
toHubTabspecial-cases four registry ids (roster,performance,dev-plans,camps) by string literal to bolt onmatchPrefixes. This duplicates route knowledge that should live alongside the registry entry itself (BaseballNavEntry) rather than as an out-of-band lookup table in the mapper function. If a fifth registry entry needs nested-route ownership, or if one of these ids is renamed, this silently stops working with no compile-time or test signal.♻️ Suggested refactor
-function toHubTab(entry: BaseballNavEntry): HubSubNavTab { - const tab: HubSubNavTab = { - id: entry.id, - label: entry.label, - href: entry.href, - icon: entry.icon, - requiredCapability: entry.requiredCapability ?? undefined, - requiredAnyCapabilities: entry.requiredAnyCapabilities, - allowedProgramTypes: entry.allowedProgramTypes, - }; - - if (entry.id === 'roster') tab.matchPrefixes = ['/baseball/dashboard/players']; - if (entry.id === 'performance') tab.matchPrefixes = ['/baseball/dashboard/performance']; - if (entry.id === 'dev-plans') tab.matchPrefixes = ['/baseball/dashboard/dev-plans']; - if (entry.id === 'camps') tab.matchPrefixes = ['/baseball/dashboard/camps']; - - return tab; -} +function toHubTab(entry: BaseballNavEntry): HubSubNavTab { + return { + id: entry.id, + label: entry.label, + href: entry.href, + icon: entry.icon, + requiredCapability: entry.requiredCapability ?? undefined, + requiredAnyCapabilities: entry.requiredAnyCapabilities, + allowedProgramTypes: entry.allowedProgramTypes, + matchPrefixes: entry.matchPrefixes, + }; +}(requires adding
matchPrefixestoBaseballNavEntryin the registry itself)🤖 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 78 - 95, The hardcoded id checks in toHubTab are duplicating route ownership and making nested-route matching fragile. Move matchPrefixes onto BaseballNavEntry in the registry itself, then have toHubTab simply copy that field into the HubSubNavTab object along with the other entry properties. Update the registry entries for roster, performance, dev-plans, and camps so the route prefixes live beside the nav metadata instead of being inferred from entry.id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/baseball/`(dashboard)/_components/hub-definitions.ts:
- Around line 187-193: Remove the redundant matchPrefixes entry from
SETTINGS_HOME_TAB in hub-definitions.ts; the tab’s href already contributes the
same path through resolve-active-hub.ts via ownedPrefixes, so keep only the href
and let the active-tab matching logic use that single source of truth.
- Around line 338-353: The COACH_MANAGEMENT_TABS supplement is keyed only by
'program-settings', so the SETTINGS_*_TAB block can disappear silently if that
base tab ever changes. Add a dev-time guard or unit test around
withSupplements/orderTabs in hub-definitions.ts that verifies the supplement key
exists in the hubEntries('management') result before applying the
SETTINGS_HOME_TAB through SETTINGS_AUDIT_TAB block, so the mismatch fails loudly
instead of dropping navigation.
In `@src/app/baseball/`(dashboard)/BaseballFairwayShell.tsx:
- Around line 573-592: The `resolvedRole` fallback in `BaseballFairwayShell` can
briefly default to `'coach'` on the `authVerified` dashboard path, causing the
wrong nav to flash for players. Update the role resolution to prefer the
synchronously available `navContext?.role` from `useBaseballNavContext()`
instead of falling back to a hardcoded coach value when `requiredRole` and
`role` are still null. Keep the existing auth gate in `BaseballFairwayShell`,
and ensure the role passed into `BaseballFairwayContent` stays aligned with the
cached session context rather than the async `useBaseballAuth` initial state.
- Around line 136-178: `playerHubToNavItem` uses a loose prefix match that can
incorrectly ակտիվate sibling routes; align it with `toNavItem` by checking
segment boundaries in the `activeMatch` logic. Update the `tabs?.some(...)` path
matching in `BaseballFairwayShell` so each `tab.matchPrefixes` comparison uses
the same `pathname === prefix || pathname.startsWith(`${prefix}/`)` pattern
already used in `toNavItem` and expected by `resolveActiveHub.ts`.
- Line 180: `RECRUITING_PROGRAM_TYPES` is duplicated in both
`BaseballFairwayShell` and `resolveActiveHub`, so move the shared set into a
single exported constant and import it from the other module. Use the existing
`RECRUITING_PROGRAM_TYPES` symbol as the source of truth, then update the hub
visibility logic and active-tab matching to reference that shared export so the
two nav paths stay aligned.
In `@src/app/baseball/`(dashboard)/dashboard/settings/page.tsx:
- Around line 94-107: Remove the duplicated settings navigation entries so each
destination is reachable from only one place. In the dashboard settings page,
update PLAYER_SETTINGS_LINKS and CONSOLIDATED_SETTINGS_LINKS to avoid re-adding
links that already exist as standalone cards, especially the privacy and
notification destinations. Use the existing settings card definitions and the
related link constants to keep a single source of truth for each settings page.
- Around line 259-276: The settings link card in the dashboard page uses
forbidden white background utilities outside the UI layer, so update the Link
styling in the CONSOLIDATED_SETTINGS_LINKS map render to replace bg-white/60 and
hover:bg-white with an approved surface treatment such as a glass utility,
bg-cream-* or a token-backed bg-surface-* class. Keep the rest of the card
structure unchanged and make the adjustment in the same Link/card block that
renders item.icon, item.label, and item.description.
- Around line 263-274: The consolidated tile links in the settings page lack a
visible keyboard focus indicator, unlike the interactive grid cards. Update the
raw Link-based tiles in the settings page so they include focus-visible styling
consistent with the Card variant="interactive" pattern, using the same kind of
focus-visible ring classes on the Link className. Locate the Link markup that
renders item.href/item.label/item.description and add a clear focus ring for
keyboard users.
In `@src/components/fairway/app-shell/FairwayBottomNav.tsx`:
- Around line 74-79: The bottom nav active state should match the sidebar’s
`NavItem.active` handling, since `FairwayBottomNav` currently uses `item.active
|| ...` and can override an explicit `false` value on route matches. Update the
active calculation in `FairwayBottomNav` to follow the same nullish-coalescing
pattern used in `FairwaySidebar`, so `NavItem.active` only falls back to
`activeMatch`/`matchActive` when it is unset and not when it is explicitly
false. Use the `items.map` block and the `active` assignment in
`FairwayBottomNav` as the target for the fix.
---
Outside diff comments:
In `@src/app/baseball/`(dashboard)/_components/hub-definitions.ts:
- Around line 78-95: The hardcoded id checks in toHubTab are duplicating route
ownership and making nested-route matching fragile. Move matchPrefixes onto
BaseballNavEntry in the registry itself, then have toHubTab simply copy that
field into the HubSubNavTab object along with the other entry properties. Update
the registry entries for roster, performance, dev-plans, and camps so the route
prefixes live beside the nav metadata instead of being inferred from entry.id.
🪄 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: 32013dbd-9357-422a-9bd0-8c091c15ced0
📒 Files selected for processing (39)
scripts/__tests__/baseball-stale-route-links.test.mjssrc/app/baseball/(coach-dashboard)/coach/college/page.tsxsrc/app/baseball/(coach-dashboard)/coach/high-school/page.tsxsrc/app/baseball/(coach-dashboard)/coach/juco/page.tsxsrc/app/baseball/(coach-dashboard)/coach/showcase/page.tsxsrc/app/baseball/(coach-dashboard)/coach/template.tsxsrc/app/baseball/(dashboard)/BaseballFairwayShell.tsxsrc/app/baseball/(dashboard)/_components/hub-definitions.tssrc/app/baseball/(dashboard)/_components/resolve-active-hub.tssrc/app/baseball/(dashboard)/dashboard/settings/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/new/NewGameClient.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/new/error.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/new/loading.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/new/page.tsxsrc/app/baseball/(dashboard)/dashboard/team/high-school/error.tsxsrc/app/baseball/(dashboard)/dashboard/team/high-school/page.tsxsrc/app/baseball/(dashboard)/layout.tsxsrc/app/baseball/(player-dashboard)/player/college/page.tsxsrc/app/baseball/(player-dashboard)/player/high-school/page.tsxsrc/app/baseball/(player-dashboard)/player/juco/page.tsxsrc/app/baseball/(player-dashboard)/player/layout.tsxsrc/app/baseball/(player-dashboard)/player/passport/page.tsxsrc/app/baseball/(player-dashboard)/player/showcase/page.tsxsrc/app/baseball/actions/__tests__/route-shell-contract.test.tssrc/app/baseball/actions/__tests__/save-full-box-score.test.tssrc/app/baseball/actions/games.tssrc/components/baseball/games/GameCard.tsxsrc/components/fairway/app-shell/FairwayBottomNav.tsxsrc/components/fairway/app-shell/FairwaySidebar.tsxsrc/contracts/baseball/stats/pitching-invariants.test.tssrc/lib/baseball/__tests__/resolve-active-hub.test.tssrc/lib/baseball/__tests__/settings-aliases-and-legacy-redirects.test.tssrc/lib/baseball/__tests__/stats-route-aliases.test.tssrc/lib/baseball/nav-manifest.tssrc/lib/baseball/nav-registry.tssrc/lib/baseball/route-contract.tssrc/lib/baseball/stats-route-aliases.tssrc/lib/supabase/middleware.tssrc/test/helpers/baseball-route-inventory.ts
💤 Files with no reviewable changes (15)
- src/app/baseball/(player-dashboard)/player/showcase/page.tsx
- src/app/baseball/(dashboard)/dashboard/stats/games/new/loading.tsx
- src/app/baseball/(coach-dashboard)/coach/high-school/page.tsx
- src/app/baseball/(dashboard)/dashboard/stats/games/new/NewGameClient.tsx
- src/app/baseball/(player-dashboard)/player/college/page.tsx
- src/app/baseball/(dashboard)/dashboard/team/high-school/page.tsx
- scripts/tests/baseball-stale-route-links.test.mjs
- src/app/baseball/(dashboard)/dashboard/team/high-school/error.tsx
- src/app/baseball/(coach-dashboard)/coach/juco/page.tsx
- src/app/baseball/(dashboard)/dashboard/stats/games/new/page.tsx
- src/app/baseball/(coach-dashboard)/coach/college/page.tsx
- src/app/baseball/(player-dashboard)/player/high-school/page.tsx
- src/app/baseball/(player-dashboard)/player/juco/page.tsx
- src/app/baseball/(coach-dashboard)/coach/showcase/page.tsx
- src/app/baseball/(dashboard)/dashboard/stats/games/new/error.tsx
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+75
to
+82
| const res = await fetch(`https://api.github.com/repos/${owner}/${repo}/issues/${issueNumber}`, { | ||
| method: 'PATCH', | ||
| headers: { | ||
| ...githubIssuesHeaders(token), | ||
| 'content-type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ labels }), | ||
| }); |
| }; | ||
|
|
||
| function stripHtmlComments(text: string): string { | ||
| return text.replace(/<!--[\s\S]*?-->/g, ''); |
| return Promise.resolve(cachedContext); | ||
| } | ||
|
|
||
| if (userId === null) { |
Wrap logServerError in Promise.resolve + try/catch so incomplete test mocks and sync throws never break action results. Lower deprecated-entry sanity threshold now that coach/player type redirects live in middleware.
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
src/app/baseball/(dashboard)/BaseballFairwayShell.tsx (1)
168-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLoose prefix match still inconsistent with
resolveActiveHub.ts's segment-boundary matching.
tab.matchPrefixes?.some((prefix) => pathname.startsWith(prefix))matches on a bare string prefix. A sibling route sharing the same string prefix (e.g./baseball/dashboard/liftvs./baseball/dashboard/lift-report) will incorrectly light up this item'sactiveMatch, same as flagged previously.🐛 Proposed fix
activeMatch: (pathname) => pathname === href || Boolean( tabs?.some( - (tab) => pathname === tab.href || tab.matchPrefixes?.some((prefix) => pathname.startsWith(prefix)), + (tab) => + pathname === tab.href || + tab.matchPrefixes?.some((prefix) => pathname === prefix || pathname.startsWith(`${prefix}/`)), ), ),🤖 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)/BaseballFairwayShell.tsx around lines 168 - 174, The activeMatch logic in BaseballFairwayShell is using a loose pathname.startsWith(prefix) check for tab.matchPrefixes, which can incorrectly mark sibling routes as active. Update the activeMatch callback to use the same segment-boundary-aware matching approach as resolveActiveHub.ts so only true route descendants match, and keep the href equality check plus tabs?.some(...) behavior intact while changing the prefix comparison inside tabs matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/admin/_components/admin-nav.ts`:
- Line 34: Admin shortcut matching is case-sensitive because `AdminShell` passes
raw `e.key` into the lookup while `admin-nav` stores shortcut keys as uppercase
in the nav config and match logic. Normalize the key consistently by uppercasing
the input in `AdminShell` before lookup or by normalizing both sides inside the
admin navigation shortcut matcher used by the nav config entries. Use the
existing `AdminShell` key handler and the admin nav shortcut fields (`key`
values like the Work log entry) as the place to apply the fix.
In `@src/app/admin/ben-leah/actions.ts`:
- Around line 80-112: The updateBenLeahIssueWorkflow mutation updates GitHub
labels but does not invalidate the cached Ben + Leah issues data, so the page
can show stale labels after success. After the setBenLeahIssueWorkflow write
succeeds, call revalidatePath for the admin Ben + Leah issues page from this
server action file, and add the next/cache import if needed. Keep the existing
validation and error handling intact, and ensure the cache refresh happens only
after the mutation completes successfully.
In `@src/app/admin/ben-leah/BenLeahIssueTable.tsx`:
- Line 1: Remove the unnecessary client boundary from BenLeahIssueTable so it
can render as a Server Component. The file-level 'use client' directive is the
issue; delete it from BenLeahIssueTable.tsx and keep IssueRow and
BenLeahIssueTable as plain render-only components since they do not use hooks,
browser APIs, or event handlers. Leave BenLeahIssueWorkflowSelect as the
existing client boundary.
In `@src/app/admin/errors/page.tsx`:
- Around line 166-171: The “Open 7-day view” link in the admin errors page is
using a dead ternary and always resolves to the same URL, so it drops all active
filters instead of preserving them. Update the Link in the errors page JSX to
reuse the existing filter-building logic used for `chipHref`/`clearParamHref`,
and construct a `wider7dHref` that keeps `filters.sport`, `severity`, `source`,
and `feature` while only overriding `window` to 168. Ensure the `Link` for “Open
7-day view” uses that new href rather than a hardcoded query string.
In `@src/app/baseball/`(dashboard)/layout.tsx:
- Around line 89-99: The dashboard layout is always passing authVerified as true
to the shell components, which can render the real shell for unauthorized
sessions. Update the shell selection in layout.tsx so both BaseballFairwayShell
and BaseballShellLayout receive authVerified from the actual authorized state,
using the authorized value in the layout condition and props. Make sure the
inner loading/unauthorized guard in BaseballShellLayout and BaseballFairwayShell
still blocks rendering until authorization is confirmed.
In `@src/hooks/use-baseball-auth.ts`:
- Around line 82-89: The `useBaseballAuth` flow is over-fetching the `users`
record by calling `supabase.from('users').select('*')` and then casting it to
`User`, which can pull unnecessary fields into `useAuthStore`. Update the query
to select only the columns actually needed by `User` and the downstream
`store.setUser`/`userRecord` usage, then keep the `maybeSingle()` handling and
`userRecord?.role` logic intact. Use the `useBaseballAuth` hook, `userResult`,
and `store.setUser` as the main anchors when narrowing the selected fields.
In `@src/hooks/use-baseball-nav-context.ts`:
- Around line 64-71: `hasWarmNavCache` is treating a legitimate resolved null
from `getBaseballNavContext` as a cache miss. Update the caching logic in
`use-baseball-nav-context` so “resolved” is tracked separately from
`cachedContext` being non-null, and let teamless users reuse the cached null
result within `NAV_CACHE_TTL_MS`. In the resolve-commit path where
`cachedContext`, `cacheResolvedAt`, and related state are updated, also mark a
dedicated resolved flag (for example alongside the existing cache user/timestamp
state) and have `hasWarmNavCache` check that flag instead of `cachedContext !==
null`.
In `@src/lib/admin/ben-leah-issues.ts`:
- Around line 92-133: The admin issues fetch path can hang because the fallback
GitHub search request in fetchRawIssues does not use the same timeout signal as
the primary fetch. Update the searchRes fetch call to reuse the existing abort
signal/timeout wiring already used for the main GitHub issues request, so both
requests are bounded consistently.
In `@src/lib/admin/data/errors.ts`:
- Around line 121-146: The 7-day fallback count queries in errors.ts only apply
the sport filter, so the empty-state hint can ignore the active filter set.
Update the wider-window logic around widerQuery and widerUntaggedQuery to
propagate the current severity, source, and feature filters as well, matching
the same filter criteria used by the main admin_events query. Keep the change
localized to the fallback count construction so the “unresolved incidents in the
last 7 days” copy in the admin errors page reflects the active filters.
In `@src/lib/admin/data/overview.ts`:
- Around line 140-142: The `sentryUnresolved` KPI in `overview.ts` is being
computed from the length of the paged `incidentFeed24h.sentry.data` array, which
only reflects the limited results returned by `fetchSentryIssues` and
`sentry-api` pagination. Update the `incidentFeed24h`/`sentryUnresolved` path to
use a dedicated unresolved total or count from the Sentry API instead of
`data?.length`, so the admin overview reports the full org-wide unresolved
number. Locate the fix in `overview.ts` where `sentryUnresolved` is assigned,
and in the `incident-feed`/`sentry-api` flow that currently fetches paged issue
data.
In `@src/lib/admin/github-issues-workflow.ts`:
- Around line 60-88: The label update in setBenLeahIssueWorkflow is PATCHing a
full label set from stale client-supplied currentLabels, which can overwrite
concurrent label changes. Change the server action to fetch the issue’s latest
labels immediately before calling applyWorkflowSelection and build the PATCH
from that fresh state instead of the browser snapshot. Then update
updateBenLeahIssueWorkflow and the BenLeahIssueWorkflowSelect call site to stop
passing currentLabels now that the action reads labels server-side.
- Around line 20-58: `ensureBenLeahGitHubLabels` is doing uncached label
bootstrapping on every call, and `createLabelIfMissing` uses an unbounded
`fetch`, so a slow GitHub API can block hot paths like `fetchBenLeahIssueBoard`,
`createGitHubFeedbackIssue`, and `setBenLeahIssueWorkflow`. Add a timeout to the
`createLabelIfMissing` request, and memoize successful completion of
`ensureBenLeahGitHubLabels` at module scope so repeated invocations in the same
warm instance skip the label-creation sequence when it has already succeeded.
In `@src/lib/admin/github-pr-timeline.ts`:
- Around line 103-129: The GitHub API requests in the pull request timeline
fetch path have no timeout, so a stalled response can block the admin render
indefinitely. Update the fetch logic in github-pr-timeline’s request flow to use
an AbortController with a short timeout for both the search and fallback list
requests, and ensure the timeout error is handled by the existing failure path
so the admin UI can fall back cleanly instead of hanging.
- Around line 92-101: The GitHub search query in github-pr-timeline uses literal
plus signs in authorQuery, which URLSearchParams encodes as %2B and prevents
GitHub from treating the qualifiers as separate terms. Update the query
construction in the timeline search logic so author filters are joined with
spaces instead of pluses, keeping the q value in the expected repo:... is:pr
author:... format and preserving the existing author filtering behavior in the
search API.
In `@src/lib/admin/observe-action-result.ts`:
- Around line 74-105: The issue is that observeActionSoftFailure can throw
synchronously and accidentally flip a successful action into a failure. Wrap the
body of observeActionSoftFailure in its own try/catch so any errors from
extractActionSoftFailure, shouldEmit, drainCollapsedCount,
severityForSoftFailure, or logServerError cannot escape. Keep the guard local to
observeActionSoftFailure in src/lib/admin/observe-action-result.ts and
swallow/log internal observability errors without affecting the calling action
paths in with-lifting-action, with-baseball-action, or observed-action.
In `@src/lib/admin/observed-action.ts`:
- Around line 78-100: The happy path in withAdminObserved is doing an
unnecessary auth lookup because resolveObservedUser() is awaited before
observeActionSoftFailure can determine whether the result is a soft failure.
Move the user enrichment behind a soft-failure check in the observed-action flow
so createClient()/supabase.auth.getUser() only run when
extractActionSoftFailure(result) indicates it is needed; keep the existing
context extraction and observeActionSoftFailure call in observed-action.ts, but
avoid resolving the user for normal successful results.
---
Duplicate comments:
In `@src/app/baseball/`(dashboard)/BaseballFairwayShell.tsx:
- Around line 168-174: The activeMatch logic in BaseballFairwayShell is using a
loose pathname.startsWith(prefix) check for tab.matchPrefixes, which can
incorrectly mark sibling routes as active. Update the activeMatch callback to
use the same segment-boundary-aware matching approach as resolveActiveHub.ts so
only true route descendants match, and keep the href equality check plus
tabs?.some(...) behavior intact while changing the prefix comparison inside tabs
matching.
🪄 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: 372f52ba-6dd1-402d-bbec-d3d1bcd6a965
📒 Files selected for processing (51)
.env.examplesrc/app/admin/_components/AdminShell.tsxsrc/app/admin/_components/__tests__/admin-nav.test.tssrc/app/admin/_components/admin-nav.tssrc/app/admin/ben-leah/BenLeahIssueBoard.tsxsrc/app/admin/ben-leah/BenLeahIssueTable.tsxsrc/app/admin/ben-leah/BenLeahIssueWorkflowSelect.tsxsrc/app/admin/ben-leah/actions.tssrc/app/admin/ben-leah/page.tsxsrc/app/admin/errors/page.tsxsrc/app/admin/page.tsxsrc/app/admin/work/WorkTimeline.tsxsrc/app/admin/work/page.tsxsrc/app/baseball/(dashboard)/BaseballFairwayShell.tsxsrc/app/baseball/(dashboard)/dashboard/settings/page.tsxsrc/app/baseball/(dashboard)/layout.tsxsrc/app/baseball/actions/__tests__/complete-player-onboarding.test.tssrc/app/baseball/actions/onboarding.tssrc/app/golf/actions/__tests__/coverage-contract.observability.test.tssrc/components/auth/baseball-sign-in-form.tsxsrc/components/baseball/BaseballDashboardBootstrap.tsxsrc/components/baseball/BaseballShellLayout.tsxsrc/components/providers/SessionActivityProvider.tsxsrc/hooks/__tests__/use-baseball-auth.test.tsxsrc/hooks/use-baseball-auth.tssrc/hooks/use-baseball-nav-context.tssrc/lib/__tests__/server-error-logger-bridge.test.tssrc/lib/admin/__tests__/ben-leah-issues.test.tssrc/lib/admin/__tests__/observe-action-result.test.tssrc/lib/admin/__tests__/pr-body-parser.test.tssrc/lib/admin/ben-leah-issue-tracker.tssrc/lib/admin/ben-leah-issues.tssrc/lib/admin/data/__tests__/incident-feed.test.tssrc/lib/admin/data/errors.tssrc/lib/admin/data/incident-feed.tssrc/lib/admin/data/overview.tssrc/lib/admin/data/triage.tssrc/lib/admin/github-feedback.tssrc/lib/admin/github-issues-config.tssrc/lib/admin/github-issues-workflow.tssrc/lib/admin/github-pr-timeline.tssrc/lib/admin/observe-action-result.tssrc/lib/admin/observed-action.tssrc/lib/admin/pr-body-parser.tssrc/lib/auth/session-activity.tssrc/lib/auth/session-idle-shared.tssrc/lib/baseball/__tests__/with-baseball-action-observability.test.tssrc/lib/baseball/with-baseball-action.tssrc/lib/lifting/with-lifting-action.tssrc/lib/server-error-logger.tssrc/test/lib/auth/session-idle-shared.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/admin/observe-action-result.ts (1)
74-111: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTry/catch still doesn't cover the whole function body — partial fix of prior finding.
The previous review flagged that
extractActionSoftFailure,shouldEmit,drainCollapsedCount, andseverityForSoftFailurecan throw synchronously and must be guarded, since this function runs inside the outertryblocks ofwith-lifting-action.ts,with-baseball-action.ts, andobserved-action.ts. The newtry { ... } catchat Lines 88-110 only wraps thelogServerErrorcall —extractActionSoftFailure(result)(Line 78) andshouldEmit/drainCollapsedCount(Lines 81-86) still execute outside any guard. SincelogServerErrorisasync, it was already implicitly safe via.catch(() => {}); the calls that actually needed protection are still exposed.🩺 Proposed fix — move the try/catch to wrap the whole function body
export function observeActionSoftFailure( result: unknown, context: ActionSoftFailureContext, ): void { - const failure = extractActionSoftFailure(result); - if (!failure) return; - - const throttleKey = `soft:${context.action}:${failure.code ?? failure.message.slice(0, 120)}`; - if (!shouldEmit(throttleKey)) return; - - const collapsedCount = drainCollapsedCount(throttleKey); - const expected = isExpectedSoftFailureMessage(failure.message); - const severity = severityForSoftFailure(failure.message); - try { + const failure = extractActionSoftFailure(result); + if (!failure) return; + + const throttleKey = `soft:${context.action}:${failure.code ?? failure.message.slice(0, 120)}`; + if (!shouldEmit(throttleKey)) return; + + const collapsedCount = drainCollapsedCount(throttleKey); + const expected = isExpectedSoftFailureMessage(failure.message); + const severity = severityForSoftFailure(failure.message); + void Promise.resolve( logServerError( failure.message, { ...context, title: `[${context.action}] ${failure.message}`.slice(0, 500), handled: true, skipSentry: expected, errorCode: failure.code ?? undefined, fingerprint: ['server_action_soft', context.feature ?? context.featureArea ?? 'unknown', context.action], metadata: { ...(context.metadata ?? {}), soft_failure: true, ...(collapsedCount > 0 ? { collapsed_count: collapsedCount } : {}), }, }, severity, ), ).catch(() => {}); } catch { // Fire-and-forget: observability must never change action results. } }🤖 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/admin/observe-action-result.ts` around lines 74 - 111, Wrap the entire observeActionSoftFailure flow in a single try/catch instead of only guarding the logServerError call. The synchronous calls that can throw—extractActionSoftFailure, shouldEmit, drainCollapsedCount, and severityForSoftFailure—must be inside the protected block so this helper remains fire-and-forget when invoked from with-lifting-action, with-baseball-action, and observed-action. Keep the logServerError promise handling as-is, but move the surrounding guard to cover all logic in observeActionSoftFailure.
🤖 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.
Duplicate comments:
In `@src/lib/admin/observe-action-result.ts`:
- Around line 74-111: Wrap the entire observeActionSoftFailure flow in a single
try/catch instead of only guarding the logServerError call. The synchronous
calls that can throw—extractActionSoftFailure, shouldEmit, drainCollapsedCount,
and severityForSoftFailure—must be inside the protected block so this helper
remains fire-and-forget when invoked from with-lifting-action,
with-baseball-action, and observed-action. Keep the logServerError promise
handling as-is, but move the surrounding guard to cover all logic in
observeActionSoftFailure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0bfeeba3-8453-4796-99fb-adb4ad5f5029
📒 Files selected for processing (3)
src/app/baseball/actions/__tests__/demo-access.test.tssrc/lib/admin/observe-action-result.tssrc/lib/baseball/__tests__/nav-manifest.test.ts
Exclude feature-registry metadata from stat-layer scan (table names only, not a DB consumer). Use structured console.error objects to satisfy CodeQL log-injection rules in server-error-logger.
Owner
Author
|
@coderabbitai full review @coderabbitai resolve |
- Move matchPrefixes onto BaseballNavEntry in nav-registry (roster, performance, dev-plans, camps); toHubTab copies verbatim - Drop redundant SETTINGS_HOME_TAB matchPrefixes; add dev guard for management settings supplement keyed on program-settings - Prefer navContext.role for resolvedRole; segment-boundary prefix matching; export shared RECRUITING_PROGRAM_TYPES from resolve-active-hub - Settings hub: dedupe privacy/notification cards, add focus ring on consolidated tiles - FairwayBottomNav: nullish-coalesce active state like FairwaySidebar
There was a problem hiding this comment.
Actionable comments posted: 1
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)/dashboard/settings/page.tsx (1)
31-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the coach settings route map
src/app/baseball/(dashboard)/dashboard/settings/page.tsx:31-92hardcodes the same coach-settings routes/icons that are already declared insrc/app/baseball/(dashboard)/_components/hub-definitions.ts:28-83(season,philosophy,roles,permissions,teams,imports,integrations,audit). Extract the shared route/icon/capability map and let this page render its own labels/descriptions from that source so route or permission changes don’t drift.🤖 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/settings/page.tsx around lines 31 - 92, The coach settings page is duplicating the same route/icon/capability definitions already kept in hub-definitions, which can drift over time. Refactor page.tsx to consume the shared settings map from hub-definitions and derive the page’s labels/descriptions from that source, using the existing symbols like COACH_SETTINGS_LINKS and the hub definition entries for season, philosophy, roles, permissions, teams, imports, integrations, and audit.src/app/baseball/(dashboard)/_components/resolve-active-hub.ts (1)
200-210: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGate the player recruiting hub on activation
src/app/baseball/(dashboard)/_components/resolve-active-hub.ts:94-120,200-210always includesplayerHubs()and returnsPLAYER_RECRUITING_TABSunchanged for players, so the recruiting strip still resolves on player routes.- The only activation gate is
src/hooks/use-route-protection.ts:31-50, and it is only wired insrc/app/baseball/(dashboard)/dashboard/college-interest/CollegeInterestClient.tsx:190-193;src/app/baseball/(dashboard)/dashboard/journey/page.tsx:187-225andsrc/app/baseball/(dashboard)/dashboard/colleges/page.tsx:23-124do not apply it.- Move the opt-in / college-player check into
playerHubs()or filterPLAYER_RECRUITING_TABSbefore returning fromresolveActiveHub().🤖 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/resolve-active-hub.ts around lines 200 - 210, The player recruiting hub is still being resolved for player routes because resolveActiveHub() always includes playerHubs() and returns PLAYER_RECRUITING_TABS without checking activation. Update playerHubs() or the resolveActiveHub() flow so the opt-in/college-player gate is applied before PLAYER_RECRUITING_TABS can be selected, and make sure the filtered tabs are excluded for non-activated players while preserving the existing coach tab filtering logic.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/server-error-logger.ts`:
- Line 330: The persist-failure log in ServerErrorLogger is leaking PII because
it prints the full enriched context. Update the failure handling around the
failed trace persistence in ServerErrorLogger to log only a minimal redacted
shape instead of the full enriched object. Use the existing normalizeContext and
enriched context flow as the place to trim or omit userId and userEmail before
the console.error call.
---
Outside diff comments:
In `@src/app/baseball/`(dashboard)/_components/resolve-active-hub.ts:
- Around line 200-210: The player recruiting hub is still being resolved for
player routes because resolveActiveHub() always includes playerHubs() and
returns PLAYER_RECRUITING_TABS without checking activation. Update playerHubs()
or the resolveActiveHub() flow so the opt-in/college-player gate is applied
before PLAYER_RECRUITING_TABS can be selected, and make sure the filtered tabs
are excluded for non-activated players while preserving the existing coach tab
filtering logic.
In `@src/app/baseball/`(dashboard)/dashboard/settings/page.tsx:
- Around line 31-92: The coach settings page is duplicating the same
route/icon/capability definitions already kept in hub-definitions, which can
drift over time. Refactor page.tsx to consume the shared settings map from
hub-definitions and derive the page’s labels/descriptions from that source,
using the existing symbols like COACH_SETTINGS_LINKS and the hub definition
entries for season, philosophy, roles, permissions, teams, imports,
integrations, and audit.
🪄 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: 50304d20-41cd-4f4e-a307-95bae9845674
📒 Files selected for processing (11)
src/app/baseball/(dashboard)/BaseballFairwayShell.tsxsrc/app/baseball/(dashboard)/_components/hub-definitions.tssrc/app/baseball/(dashboard)/_components/hub-sub-nav.tsxsrc/app/baseball/(dashboard)/_components/resolve-active-hub.tssrc/app/baseball/(dashboard)/dashboard/settings/page.tsxsrc/components/fairway/app-shell/FairwayBottomNav.tsxsrc/components/layout/sidebar.tsxsrc/lib/baseball/__tests__/resolve-active-hub.test.tssrc/lib/baseball/nav-registry.tssrc/lib/baseball/stat-layer-manifest.tssrc/lib/server-error-logger.ts
|
The PR description images show two dark-themed pill-shaped buttons: the first reads "Open in Web" with an arrow icon pointing up/right, and the second reads "Open in Cursor" with the Cursor logo icon. Both are standard "open in editor" link controls unrelated to the code review itself.
✅ Action performedComments resolved and changes approved. Full review finished. |
|
Too many files changed for review. ( Bypass the limit by tagging |
Use static Sentry capture titles and synthetic Error shells so user/error copy flows through scope context (trace_message) instead of format strings. Renames console payload keys to traceMessage for log-injection hygiene.
|
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. |
7 tasks
This was referenced Jul 4, 2026
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unified Fairway shell routing + Helm Bridge observability on one branch.
Fairway routing
activeMatch/matchPrefixeswired through route-contractHelm Bridge observability
observeActionSoftFailure), incident feed, Ben+Leah boardCodeRabbit polish (latest)
matchPrefixesmoved ontoBaseballNavEntryin nav-registry (single source of truth)navContext.role(no coach-nav flash)RECRUITING_PROGRAM_TYPESexport; management settings supplement dev guardFairwayBottomNavactive state uses nullish coalescing like sidebarCI fixes
Supersedes #786 — close that PR when this merges.