All-of-it batch: marketing root, wizard consolidation, velocity, visual-audit infra, de-vibe wave 2a, public-page motion fix - #868
Merged
Conversation
…s, insight-actions) (#858) Verified dead via grep (import path + symbol name + next/dynamic scan), then git rm. No consumers found in src/, no test coverage, no dynamic imports referencing any of these paths. - src/components/baseball/coach/ModeToggle.tsx — exports JUCOModeToggle, zero importers repo-wide. Only referenced from stale docs (PHASE_5_JUCO_COACH.md, .helm/ACTIONS.md) describing a wiring into src/components/layout/header.tsx, which no longer exists. - src/components/layout/mode-toggle.tsx — exports ModeToggle/Mode, its only consumer was the dead file above. - src/components/features/notification-center.tsx — duplicate/legacy NotificationCenter; the live one is src/components/golf/calendar/NotificationCenter.tsx. .taskmaster/docs/current-state.md already flagged it "Exists but not used". - src/hooks/use-notifications.ts — duplicate/legacy useNotifications; the live hook is src/hooks/useNotifications.ts (capital N), consumed by the real NotificationCenter. - src/components/golf/coachhelm/insights/{InsightBulkActions,InsightExportModal, InsightFiltersPanel,InsightSearchBar}.tsx — not exported from the insights/ barrel (index.ts only re-exports PlayerFocusAreas/InsightsFeed/InsightListView per its "Wave 1A" comment), zero direct importers, no next/dynamic references. - src/lib/baseball/lifting/use-live-set-sync.ts — exports useLiveSetSync, zero importers; only mentioned in docs/audits (planned-but-never-wired). Gates: typecheck clean, check-cycles clean (33 known cycles, none new), no test files reference any of these paths. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ess barrel, lift-programs) (#859) Verified dead via grep (import path + symbol name + next/dynamic scan), then git rm. - src/components/golf/travel/{ExpenseForm,ExpenseList,ExpenseSummary,index}.ts(x) — legacy pre-Fairway components. Superseded by src/components/fairway/pages/travel/ Fairway{ExpenseForm,ExpenseList,ExpenseSummary}.tsx, whose own header comments say they're re-skins of "the legacy golf/travel ExpenseList/ExpenseSummary" — i.e. the legacy files are explicitly documented as replaced. Zero live importers (grep for the barrel path and each symbol name comes back empty outside the legacy files themselves). - src/components/lifting/soreness/index.ts — barrel; zero importers (every other file in the same directory — BodySilhouetteFront, SorenessCheckCard, SorenessBodyMap, HighPrioritySorenessList, SorenessScheduleBuilder — IS imported directly by app code, just never through this barrel). - src/components/lifting/soreness/SorenessComplianceBoard.tsx, TeamSorenessHeatmap.tsx — only referenced from the dead barrel above; no direct importers. - src/lib/baseball/read-models/lift-programs.ts — exports getLiftProgramList/ getLiftProgramTree/getAssignContext. The live /performance/programs/[programId] page defines its own local getAssignContext (duplicated, not imported from here) — confirms this read-model was built but never wired in. Gates: typecheck clean, check-cycles clean (33 known cycles, none new). `grep` false-positive check: src/app/golf/actions/__tests__/travel.test.ts matches "ExpenseSummary" only via the substring in getExpenseSummary() (a server action, unrelated file) — ran that suite standalone to confirm (128 passed, 4 skipped, unaffected). Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ature, stray App Store Connect snapshots) (#860) - .taskmaster/ (9 tracked files: README, config.json, docs/current-state.md, docs/feature-checklist.md, docs/prd.txt, logs/.gitkeep, state.json, tasks/tasks.json, templates/task-template.json) — task-master scaffolding from an abandoned tool integration. Only appears elsewhere as ignore-list entries (.gitignore:76-77), never read by any script/workflow/package.json script. Zero functional references. - .full-stack-feature/ (2 tracked files: 01-requirements.md, state.json) — same pattern: only appears as ignore-list entries across .gitignore, .coderabbitignore, .coderabbit.yaml, .vercelignore, .greptile/config.json, .greptile/rules.md (all just telling other tools to skip the directory). Zero functional references. - full-snapshot.yml, full-snapshot2.yml, app-info-snapshot.yml, age-ratings-snapshot.yml — accessibility-tree/DOM snapshots of the App Store Connect web UI (not fastlane config — there is no fastlane/ directory anywhere in this repo, which uses Xcode Cloud, not fastlane). Zero script or CI references (grepped scripts/, tools/, .github/, .circleci/ — nothing reads these paths). The one doc mention (docs/operations/2026-05-28-coderabbit-fails-investigation.md) explicitly calls age-ratings-snapshot.yml "INHERITED NOISE" causing ~200 yamllint indentation errors and recommends "delete it if it's truly unused" — it is. review-gate.yml's yamllint job only lints *changed* files in a PR diff, so these aren't continuously failing CI, but they're pure accidental commits (browser-automation output) with zero purpose in the repo. - context7.json — does not exist (only context7.json.example is tracked; the real context7.json was already removed in a prior commit 6a9b565 "fix(security): stop tracking context7.json (contained leaked API key)"). Nothing to do here. Gates: typecheck clean, check-cycles clean (33 known cycles, none new). Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…vice-worker (#861) Audited the 77 console.log/debug/warn call sites in prod src (excluding tests). Two mechanisms make almost all of them deliberate, not vibe-coded leftovers, and this PR documents why nearly everything was kept: - next.config.mjs compiler.removeConsole strips console.log AND console.debug from production builds, excluding only 'error'/'warn'. So every console.log/.debug call is already dev-only/no-op in prod. - src/instrumentation.ts + src/instrumentation-client.ts both configure Sentry.consoleLoggingIntegration({ levels: ['log','warn','error'] }) — console.warn is the established, load-bearing structured-logging idiom in this codebase (forwarded to Sentry Explore → Logs), which is exactly why admin-tracer-data.ts has an explicit comment: "console.warn used (not console.log) because production build strips console.log." Reviewed every one of the 48 console.warn and 8 console.debug call sites individually: every single one has either an explicit comment justifying the log level (e.g. insight-delivery.ts's transient-fetch debug downgrade, useAdminPresence.ts's `if (process.env.NODE_ENV !== 'production')`-gated join/leave debug logs, pattern-miner.ts's documented severity policy, admin-logger.ts's PGRST205 once-only warn) or is a genuine production security/error signal (auth rate-limiting, unauthorized message/team actions, fetch-failure fallbacks). None were genuine leftovers — all kept as-is, no logger-idiom conversion performed (see below). **Deleted** (1 file, 8 statements): src/hooks/golf/use-service-worker.ts — 8 console.log calls tracing every SW lifecycle branch (register no-op, already-registered, registered, unregistered, update complete, sync unsupported, sync registered, no active worker to message, message received). Unlike every kept call site above, these had (a) no explanatory comment, (b) no dev-only guard, (c) duplicate state already exposed via the hook's own return value (`status`/`isRegistered`/ `hasUpdate`), and (d) trace literally every branch including plain early returns — the classic "log every branch while debugging a tricky SW bug" pattern (see memory: dev-SW false-offline investigation) never cleaned up. The 5 console.error calls in this same file's catch blocks are untouched (KEEP per the task rule). **Logger-idiom conversion**: grepped for a logger util first (src/lib/admin-logger.ts, server-error-logger.ts, error-logging.ts exist) — none is a general-purpose console.warn replacement; they're purpose-built for the admin_events audit trail / Sentry error classification, and console.warn already IS the repo's structured-log idiom for this class of signal (per the Sentry consoleLoggingIntegration wiring above). Converting would be redundant double-logging and risk semantic changes (async logger calls dropped into sync catch blocks) for no observability gain, so no conversions were made — warns left as-is, per the "if none, leave warns" instruction. Gates: typecheck clean, eslint --max-warnings 0 on the touched file clean, check-cycles clean (33 known cycles, none new). No test file covers this hook (grepped for use-service-worker in *.test.*/*.spec.* — zero hits). Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Signed-out visitors used to get bounced straight to /baseball/login with
zero context; they now see a real front door — hero, four editorial
feature sections (roster/team-ops, stats center, recruiting pipeline,
player passport) composed from the Living Annual kit in ghost/placeholder
state (no fabricated screenshots or invented player data), and an honest
CTA row (Sign in / Create a program / Join with a code). Signed-in
visitors keep the exact prior redirect-to-dashboard behavior.
- src/app/baseball/page.tsx: rewritten from a bare redirect into the full
marketing page; auth check now only fires the redirect when a session
exists.
- src/components/baseball/marketing/BaseballMarketingMotionScope.tsx: new
tiny 'use client' LazyMotion wrapper — the Living Annual atoms used here
(RuledStatLine/Masthead/HairlineRule/GradeStamp) never transition off
their hidden variant without a loaded feature bundle, and the page
itself stays a Server Component (async session check + redirect), so
this is the one client boundary.
- src/app/baseball/join/page.tsx: new — the "Join with a code" CTA needed
a real destination; only the dynamic /baseball/join/[code] existed.
Mirrors GolfHelm's /golf/join code-entry page, themed in the Living
Annual paper/ink system instead of golf's glass-orb auth chrome.
- src/components/landing/Footer.tsx: generalized the shared cross-product
footer's tagline off golf-only wording ("college golf") since it now
also renders under a BaseballHelm hero.
- src/app/baseball/__tests__/page.test.tsx: pins the redirect/no-redirect
branching (coach session, player session, signed-out).
Co-authored-by: Fable Integrator <fable@helm.local>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…yMotion ancestor) (#866) team/[id], player/[id] (via PlayerProfileClient), program/[id], and packet/[token] sit in the (public) route group, whose layout was a bare `<>{children}</>` — no LazyMotion anywhere upstream. team/[id] and PlayerProfileClient render Living Annual `m`-based atoms (Masthead, RuledStatLine, HairlineRule) directly; their `inkSettles`/`rulesDraw` entrance variants start at `hidden` (opacity: 0 / scaleX: 0) and only animate to `visible` once framer-motion's feature bundle is loaded via a `LazyMotion` ancestor. Without one, an `m.*` component's AnimationFeature never mounts, so the hidden variant is terminal for any visitor without `prefers-reduced-motion` on — player/team names and stat numerals stayed invisible on these live public recruiting pages. Adds PublicMotionScope (mirrors the existing AdminMotionProvider / `(dashboard)/dashboard/template.tsx` pattern already used elsewhere in the repo) and mounts it from `(public)/layout.tsx`, which stays a Server Component — the LazyMotion boundary lives in the client child. Verified via a real (unmocked) framer-motion render test: Masthead's surname text is measurably opacity: 0 forever with no wrapper, and measurably transitions off 0 once PublicMotionScope loads its feature bundle — the same computed-opacity check `toBeVisible()` uses, so it reproduces the actual bug and the actual fix rather than a mocked stand-in. program/[id] and packet/[token] don't currently render any Living Annual `m` atoms directly (packet's ScoutPacketView already carries its own LazyMotion) — the shared layout-level provider covers them defensively against regression as those pages grow. PR #865 (open, targets this same base) adds a near-identical BaseballMarketingMotionScope for the separate /baseball marketing root and explicitly flagged this (public) route group gap out of its own scope; this PR is the fix for that flagged gap. Not touching #865's files — noted in the PR body that the two wrappers could be consolidated into one shared component later. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
New e2e/visual-audit.spec.ts mirrors baseball-route-crawler.spec.ts's proven live-DOM nav discovery (FairwaySidebar + hub-sub-nav <nav> links) and best-effort public-sample-link discovery, but captures full-page screenshots at phone (390x844) and desktop (1440x900) viewports for every discovered coach/player route plus signed-out publics, instead of asserting route health. Screenshots are data capture, not assertions — the spec only fails on a login failure or a total navigation failure. Gated behind VISUAL_AUDIT=1 (test.skip otherwise); playwright.config.ts's chromium project now ignores it and baseball-coach/baseball-player now match it, so it never runs in the ordinary e2e lane and playwright.yml/ci.yml (which name their spec files explicitly) never pick it up. New .github/workflows/visual-audit.yml runs it via workflow_dispatch against a chosen base_url (default prod), --project=baseball-coach --project=baseball-player only — verified against the installed Playwright runner source that this also runs the `setup` project's full baseball auth (both roles) as a dependency, without needing an explicit --project=setup, and without ever touching Golf's auth.setup.ts. Uploads test-results/visual-audit as visual-audit-<run_number>, if: always(). Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…k's go) (#862) * db(baseball): write #379 legacy stats backfill migration (pending Nick's go) One-time, NOT-APPLIED migration that copies legacy baseball_player_stats 'game' rows into baseball_box_score_batting/_pitching + synthesizes shared baseball_games rows, scoped to teams with ZERO existing box-score data (teams already on the box-score adapter path are never touched). Deterministic ids (SHA-1, RFC4122-v5-shaped, own namespace) mirror #827's scripts/seed-baseball-stats.mjs detId() pattern so re-applying is a no-op and rollback can recompute — not just look up — exactly which rows are ours. Copy-only: legacy rows are never mutated. Deliberately skips recalculate_baseball_season_stats() to avoid clobbering any pre-existing season_totals-imported baseline on baseball_player_season_stats — documented as an opt-in follow-up instead. Exercised end-to-end against a disposable local Postgres 16 instance (schema mirrored from the real migrations, never any shared project) covering a two-way partial-innings player, a duplicate-row collision, an already-box-score team (excluded), and a pre-existing-scheduled-game collision (date skipped) — verified idempotent re-run and a dry-run rollback recompute+delete. See docs/baseball/legacy-backfill-runbook.md for the check-first queries, apply steps, and rollback recipe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): make #379 backfill's season-stats safety story true, not just written Adversarial review on PR #862 found the migration's core safety claim false: recalculate_baseball_season_stats() is described as a deliberate, manual, opt-in, per-team step, but the already-shipped save_baseball_full_box_score RPC calls it automatically on every ordinary box-score save. Since the backfilled games carry their real historical game_date (plausibly within the current season year for teams whose whole history predates #827), the very next normal game entry for an overlapping player would silently overwrite baseball_player_season_stats -- including any pre-existing season_totals baseline -- with no opt-in and no signoff. Fix, verified against a disposable local Postgres 16 instance (never any shared Supabase project): - Migration: add Step 4, seeding baseball_player_season_stats for exactly the (player_id, team_id, season_year) triples the migration's own box-score rows touch, using the identical aggregation/rate formulas recalculate_baseball_season_stats() uses -- guarded by ON CONFLICT ... DO NOTHING so a pre-existing row (e.g. a season_totals baseline) is never touched, preserving copy-only/additive-only/idempotent. Where no row existed, the eventual live recalc now lands on the same numbers already seeded (a no-op, not a surprise). - Runbook: replace the "deliberately out of scope" framing with the true story, add a pre-flight query that surfaces exactly which triples still carry pre-existing-baseline risk (Nick must review before applying), and add a diff-based season-stats rollback procedure since DO NOTHING rows have no deterministic id to recompute against. Locally reproduced the exact scenario the review described (a fresh ordinary game save via the real, unmodified RPC): the seeded player's row extended cleanly with correct math; the pre-existing baseline player's row was silently overwritten by the (unmodified) live RPC, exactly as newly documented -- confirming the fix and the doc are both now accurate. File remains WRITE-ONLY / NOT APPLIED pending Nick's go-ahead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…-sweep/action-baseline (#852 residual) (#864) * baseball(engine): wire event-derived velocity into engine-run/outcome-sweep/action-baseline (#852 residual) Box-score-migrated players had NO velocity metrics: their legacy exit_velocity/pitch_velocity scalar is dropped alongside superseded legacy GAME rows (engine-stat-rows.ts rule 1), and the canonical box-score tables carry no velocity columns at all. loaders.ts's eventDerived hook (#851) already threaded a per-field event-layer override into loadPlayerMetrics, but nothing called it. Adds src/lib/baseball/coachhelm/engine-event-derived.ts: a team-scoped, paginated read of baseball_pitch_events/baseball_batted_ball_events (#813 superseded-row filter) plus a pure per-player reducer that reuses elite-stat-events.ts's real buildHitterMetrics/buildPitcherMetrics + loaders.ts's eventDerivedVelocityFromMetrics -- never a second, drifting "average exit velocity" implementation. All-or-nothing degrade on read failure, mirroring engine-stat-rows.ts's own honesty rule. Wires it into all three engine callers: - engine-run.ts: full-history event pool -> loadAllPlayerMetrics. - outcome-sweep.ts: event rows filtered to the SAME per-action after-window as the box-score read, so a pre-action event never counts toward did-it-move measurement. - action-baseline.ts: full-history event pool -> the baseline capture. Tests: pure aggregation (mixed hitter/pitcher, zero-event absence, supersede filter, all-or-nothing degrade) plus per-caller wiring tests (event wins over legacy scalar for the same player; a zero-event player keeps their legacy velocity; event-read failure degrades every player to legacy). Extends stat-layer-manifest.ts's grandfathered-consumer allowlist for the new fixture files (legacy baseball_player_stats rows are the fallback pin, not staleness). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): bound velocity event read to player scope + fix sampleSize honesty (PR #864 fix-first) Two adversarial-review criticals on #864: 1. buildActionOutcomeSeed (action-baseline.ts) fired a TEAM-WIDE, unbounded, player-unscoped read of the entire pitch/batted-ball event history on every coach "convert to action" click, just to resolve ONE player's velocity scalar. loadEngineEventRows now takes an optional `playerIds` scope (`.in('pitcher_id'|'batter_id', playerIds)`, mirroring loadEngineStatRows's own `.in('player_id', playerIds)` idiom) — the single-player caller passes `[playerId]`; engine-run/outcome-sweep now pass their own already-computed roster/todo player-id lists instead of reading the whole team's history. 2. avg_exit_velocity's sampleSize was `bbCount` (every batted ball) instead of the count of rows that actually carried a non-null exit_velocity reading — inflating the honesty gate for any team whose batted-ball capture doesn't always log a radar reading. Fixed to `battedBalls.filter(b => b.exit_velocity != null).length`, and applied the same fix to the sibling avg_launch_angle metric (identical bug, same line shape). Pitcher avg_velocity was already correct. Tests: pin the DB-level player scoping (loadEngineEventRows + a buildActionOutcomeSeed integration check), and pin the sampleSize fix (10 batted balls / 4 readings -> sampleSize 4; independent launch_angle gating; hard_hit_rate's bbCount-based denominator unaffected). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Consolidate stats-upload wizard into Import Center (canonical) Audited both wizards end-to-end (§3.11 decision: Import Center is canonical). Ported the two real capability gaps before retiring the legacy path — everything else (atomic save_baseball_full_box_score RPC, player-match corrections, dedup/provenance/rollback) was already covered by Import Center's commitImport pipeline, so nothing else needed porting: - ImportWizardClient: added a "Quick box score" entry point on the choose step (preselects game_box_score + jumps straight to Upload) plus drag-and-drop onto the dropzone and a sample-values data-preview table on the detect step — the legacy wizard's two capabilities Import Center didn't have. No server-action signatures changed. - /dashboard/stats/upload is now a pure redirect into /dashboard/import, mirroring the stats -> stats-center legacy-redirect shim idiom. Sibling error.tsx/loading.tsx removed (that idiom has neither). - Retired the now-fully-orphaned StatsUploadClient/UploadHistory components (only ever imported by the old page). - Repointed the two in-app links that still pointed at the legacy route (Command Center's "Upload stats", Stats Center's header) straight at Import Center, and dropped Stats Center's redundant "Upload" button (Import Center already sat right next to it, same destination). - Test migration: extended settings-aliases-and-legacy-redirects.test.ts with the new shim, added ImportWizardClient.quick-box-score.test.tsx for the two ported capabilities, and updated the e2e assertion that pinned the retired wizard's UI strings to assert the redirect instead. nav-registry.ts (frozen) still lists /baseball/dashboard/stats/upload in stats-center's matchPrefixes and STAFF_CAPABILITY_ROUTES/GUARD_ALLOWLIST still gate it at can_manage_stats — both harmless now (a plain redirect page, still resolves on disk, destination re-enforces can_manage_imports itself) but flagging for the orchestrator in case a follow-up wants them tidied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * Fix wizard-consolidation capability lockout + restore upload history (PR #863) Adversarial review (FIX_FIRST) flagged two criticals in the stats-upload -> Import Center consolidation: 1. CAPABILITY LOCKOUT — the /stats/upload redirect shim + the two repointed CTAs sent every viewer straight at Import Center's can_manage_imports gate, locking out every default staff role that holds can_manage_stats but not can_manage_imports (assistant/pitching/hitting/catching/defensive/strength coach — 6 of 11 canonical BASEBALL_STAFF_ROLE_PRESETS). Those roles could reach and interact with the old wizard before this consolidation. Fix: /stats/upload now branches on capability instead of redirecting unconditionally. can_manage_imports staff still forward to the full Import Center; can_manage_stats-only staff get the SAME ImportWizardClient rendered inline, restricted to the "Quick box score" entry point (new quickEntryOnly prop — skips the choose step and hides the "change data shape" affordance, no way to reach the full shape picker/event-level mode/source registry/rollback reserved for can_manage_imports staff). Middleware's STAFF_CAPABILITY_ROUTES already allowlists this exact route at can_manage_stats, so no middleware/nav-registry contract change was needed. Command Center's "Upload stats" and Stats Center's two CTAs are repointed from /dashboard/import back to /dashboard/stats/upload so every entry point resolves through the capability-aware router. 2. UPLOAD HISTORY DELETED — UploadHistory.tsx was the only surface reading baseball_stat_uploads (filename/status/processed counts); its deletion left every pre-consolidation upload record permanently unviewable. Fix: ported a read-only "Legacy uploads" section into ImportWizardClient (Living Annual idiom: Eyebrow/HairlineRule/EditorsLetter honest empty state, matching the existing "Recent imports" section), backed by getRecentUploads — an existing, already-demoSafe, already-team-scoped server action with zero prior callers. No server-action signature changes. Wired into both the full Import Center page and the new capability-aware /stats/upload entry point. Also extracted the roster-for-matching query (previously inlined in import/page.tsx) into a shared src/lib/baseball/import-roster.ts helper so both pages load player-matching data identically instead of drifting. Gates: typecheck clean, eslint --max-warnings 0 clean on all touched files, targeted + broader baseball vitest suites green (1178 tests), check-cycles clean (33 known cycles, none new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(stats-center): route import entry points by viewer capability The two Import Center entry points (header action + empty-state CTA) sent everyone through the /stats/upload shim, whose middleware gate is can_manage_stats — bouncing import-capable-but-not-stats staff (e.g. the director_ops preset) off middleware before the shim's own capability branch could forward them. The page now computes can_manage_imports server-side (same helper the shim branches on) and import-capable viewers go straight to /dashboard/import; everyone else keeps the shim path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball-import): authorize stats-only staff for box-score import commit/preview (PR #863 round-4) previewImport/commitImport were hard-gated to can_manage_imports unconditionally, so the quickEntryOnly inline wizard at /stats/upload (rendered for the 6 can_manage_stats-only staff presets) let a stats-only coach fill out the whole form and then fail server-side on submit. Pre-consolidation, stats-only staff could upload box scores via the legacy wizard, so restore that: a 'game_box_score' request may now be authorized by can_manage_imports OR can_manage_stats; every other shape (season_totals, event_log, or omitted) keeps the original can_manage_imports-only gate. - with-baseball-action.ts: requiredCapability now also accepts a readonly array (ANY-of) or a resolver function of the action's own args, resolved once before AUTH so tags/metadata and enforcement can never disagree. Single-capability call sites (~60 existing) resolve to a one-element list and behave byte-identically to before. - imports.ts: previewImport gained an optional dataShape field (mirroring CommitImportArgs.dataShape) so the same shape-conditional gate applies at preview time too; both actions resolve the OR-gate from the exact field applyImportPlan uses for canonical-table routing, so the auth decision and the write decision can never diverge. - ImportWizardClient.tsx: pass dataShape through to previewImport, and hide the Upload step's "Back to choose" button for quickEntryOnly viewers (it routed to the full shape picker Import Center reserves for can_manage_imports staff). - New suite (imports-capability-shape-gate.test.ts) exercises the real withBaseballAction/capabilities wiring (not a passthrough mock) to prove: stats-only + game_box_score authorizes and actually writes; stats-only + season_totals still throws BaseballCapabilityError with zero side effects; no-capability staff still denied; imports-only staff unchanged across every shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Too many files changed for review. ( Bypass the limit by tagging |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Updates to Preview Branch (batch/allofit-0715) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
… strict) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (76)
WalkthroughThe PR consolidates baseball import routing and capability checks, adds public landing and join experiences, derives velocity metrics from event data, introduces a legacy-statistics backfill migration, and adds authenticated visual-audit Playwright coverage. ChangesBaseball public experience
Capability-aware import flow
Event-derived baseball metrics
Legacy statistics backfill
Visual audit coverage
Estimated code review effort: 5 (Critical) | ~120 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 (1 error, 3 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/app/baseball/(dashboard)/dashboard/stats/upload/page.tsx (1)
65-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd route-level coverage for the stats-only branch.
The action tests validate authorization, but they do not prove this page avoids redirecting
can_manage_stats-only staff and passesquickEntryOnlyto the inline wizard. Add a page test for that capability matrix.🤖 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/stats/upload/page.tsx around lines 65 - 93, Add page-level tests for the stats upload route covering users with can_manage_stats but not can_manage_imports: verify the page renders without redirecting and passes quickEntryOnly to ImportWizardClient. Also cover the unauthorized capability case using the existing authorization test patterns, while preserving the current team, roster, and recent-upload behavior.
🤖 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 `@docs/baseball/legacy-backfill-runbook.md`:
- Around line 279-287: The rollback documentation must require an immutable
apply-time manifest rather than recomputing IDs from current
baseball_player_stats. Update the apply procedure to record exact created game
IDs and touched season triples, then have rollback delete only those recorded
games and recalculate each touched season triple from remaining canonical games,
deleting the season row only when none remain; apply the same guidance to the
related section around the referenced rollback steps.
- Around line 49-52: Update the copy-only summary in the legacy backfill runbook
to include baseball_player_season_stats alongside the three existing INSERT-only
tables. Keep the statement that the migration does not UPDATE or DELETE rows and
that the legacy table is read-only input.
In `@src/app/baseball/join/page.tsx`:
- Around line 34-49: Update handleSubmit to validate that trimmed contains only
letters and numbers before router.push, while preserving the existing empty and
minimum-length checks. Set a clear error message for invalid characters and
return without navigating; keep valid invite codes flowing to the existing
`/baseball/join/${trimmed}` route.
In `@src/components/baseball/import-center/ImportWizardClient.tsx`:
- Line 1744: Add suppressHydrationWarning to the element enclosing the
created_at date text in the import wizard user rendering, preserving the
existing conditional toLocaleDateString formatting and fallback.
In `@src/components/baseball/stats-center/StatsCenterClient.tsx`:
- Around line 83-93: Update StatsCenterClient and its import-action
rendering/routing logic to accept and use canManageStats and
canAccessImportEntry alongside canManageImports. Only expose the import actions
when the viewer has at least one relevant capability; preserve direct Import
Center routing for canManageImports users and the /stats/upload shim for
stats-capable users, while omitting actions for viewers lacking both
capabilities.
In `@src/lib/baseball/read-models/elite-stat-events.ts`:
- Around line 746-764: Filter the provenance rows passed to each scalar metric
so they include only records with a non-null value for that metric. In the
avg_exit_velocity and avg_launch_angle scalarMetric calls, replace the all-rows
bbProv provenance with filtered provenance matching each metric’s existing
sampleSize predicate, while preserving the current averages and sample counts.
In `@src/lib/baseball/with-baseball-action.ts`:
- Around line 187-190: Update the requiredCapability type and all related
handling in withBaseballAction, including the branches around the capability
resolver and requireBaseballCapability, to use a non-empty readonly tuple for
capability arrays. Validate runtime resolver results as non-empty before
indexing or requiring a capability, and reject empty arrays rather than passing
undefined to requireBaseballCapability.
- Around line 17-23: Move conditional capability resolver evaluation from before
the wrapper’s guarded flow into the guarded action block, after getUser() and
before authorization proceeds. Update the capability tags only after the
resolver successfully returns, so malformed invocations such as omitted
arguments are handled by the existing centralized logging and
BaseballActionError sanitization. Apply the same ordering to the related logic
near the other referenced capability checks.
In `@supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql`:
- Around line 171-176: Rename the temporary tables _bb_legacy_backfill_379_teams
and _bb_legacy_backfill_379_games to names using the required baseball_ prefix,
and update every DROP, CREATE, INSERT, SELECT, JOIN, and other reference
consistently throughout the migration. Preserve their temporary-table behavior
and existing logic.
- Around line 166-185: Protect the eligibility snapshots created by the
temporary tables _bb_legacy_backfill_379_teams and the corresponding snapshot
flow around lines 201-224 from concurrent writes. Before inserting either
snapshot, acquire appropriate locks on the source and destination baseball
stats/box-score tables, or enforce execution during a write-maintenance window,
so concurrent game or box-score saves cannot invalidate eligibility or produce
mixed or duplicate team-date data.
- Around line 555-595: The bb379_pit_agg aggregation currently sums X.1/X.2
notation as decimal innings, producing incorrect totals and rate stats.
Normalize bsp.ip to total outs before aggregation, sum the outs, and convert the
result back to baseball innings notation for ip; ensure bb379_pit_final’s ERA,
WHIP, K/9, and BB/9 calculations use the normalized value consistently.
---
Nitpick comments:
In `@src/app/baseball/`(dashboard)/dashboard/stats/upload/page.tsx:
- Around line 65-93: Add page-level tests for the stats upload route covering
users with can_manage_stats but not can_manage_imports: verify the page renders
without redirecting and passes quickEntryOnly to ImportWizardClient. Also cover
the unauthorized capability case using the existing authorization test patterns,
while preserving the current team, roster, and recent-upload behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ade0aa10-6f84-4ce7-bf8c-4b4b8dcf59b7
📒 Files selected for processing (78)
.full-stack-feature/01-requirements.md.full-stack-feature/state.json.github/workflows/visual-audit.yml.taskmaster/README.md.taskmaster/config.json.taskmaster/docs/current-state.md.taskmaster/docs/feature-checklist.md.taskmaster/docs/prd.txt.taskmaster/logs/.gitkeep.taskmaster/state.json.taskmaster/tasks/tasks.json.taskmaster/templates/task-template.jsonage-ratings-snapshot.ymlapp-info-snapshot.ymldocs/baseball/legacy-backfill-runbook.mde2e/baseball-stats-smoke.spec.tse2e/visual-audit.spec.tsfull-snapshot.ymlfull-snapshot2.ymlplaywright.config.tssrc/app/baseball/(dashboard)/dashboard/import/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats-center/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/upload/error.tsxsrc/app/baseball/(dashboard)/dashboard/stats/upload/loading.tsxsrc/app/baseball/(dashboard)/dashboard/stats/upload/page.tsxsrc/app/baseball/(public)/PublicMotionScope.test.tsxsrc/app/baseball/(public)/PublicMotionScope.tsxsrc/app/baseball/(public)/layout.tsxsrc/app/baseball/__tests__/page.test.tsxsrc/app/baseball/actions/__tests__/imports-capability-shape-gate.test.tssrc/app/baseball/actions/imports.tssrc/app/baseball/join/page.tsxsrc/app/baseball/page.tsxsrc/components/baseball/coach/ModeToggle.tsxsrc/components/baseball/command-center/CommandCenterFairway.tsxsrc/components/baseball/import-center/ImportCenterShell.tsxsrc/components/baseball/import-center/ImportWizardClient.tsxsrc/components/baseball/import-center/__tests__/ImportWizardClient.quick-box-score.test.tsxsrc/components/baseball/marketing/BaseballMarketingMotionScope.tsxsrc/components/baseball/stats-center/StatsCenterClient.tsxsrc/components/baseball/stats/StatsUploadClient.tsxsrc/components/baseball/stats/UploadHistory.tsxsrc/components/baseball/stats/index.tssrc/components/features/notification-center.tsxsrc/components/golf/coachhelm/insights/InsightBulkActions.tsxsrc/components/golf/coachhelm/insights/InsightExportModal.tsxsrc/components/golf/coachhelm/insights/InsightFiltersPanel.tsxsrc/components/golf/coachhelm/insights/InsightSearchBar.tsxsrc/components/golf/travel/ExpenseForm.tsxsrc/components/golf/travel/ExpenseList.tsxsrc/components/golf/travel/ExpenseSummary.tsxsrc/components/golf/travel/index.tssrc/components/landing/Footer.tsxsrc/components/layout/mode-toggle.tsxsrc/components/lifting/soreness/SorenessComplianceBoard.tsxsrc/components/lifting/soreness/TeamSorenessHeatmap.tsxsrc/components/lifting/soreness/index.tssrc/hooks/golf/use-service-worker.tssrc/hooks/use-notifications.tssrc/lib/baseball/__tests__/action-baseline-event-velocity.test.tssrc/lib/baseball/__tests__/action-baseline.test.tssrc/lib/baseball/__tests__/engine-event-derived.test.tssrc/lib/baseball/__tests__/engine-run-event-velocity.test.tssrc/lib/baseball/__tests__/outcome-sweep-event-velocity.test.tssrc/lib/baseball/__tests__/outcome-sweep-insight-resolve.test.tssrc/lib/baseball/__tests__/settings-aliases-and-legacy-redirects.test.tssrc/lib/baseball/coachhelm/action-baseline.tssrc/lib/baseball/coachhelm/engine-event-derived.tssrc/lib/baseball/coachhelm/engine-run.tssrc/lib/baseball/coachhelm/outcome-sweep.tssrc/lib/baseball/import-roster.tssrc/lib/baseball/lifting/use-live-set-sync.tssrc/lib/baseball/read-models/__tests__/elite-stat-events.test.tssrc/lib/baseball/read-models/elite-stat-events.tssrc/lib/baseball/read-models/lift-programs.tssrc/lib/baseball/stat-layer-manifest.tssrc/lib/baseball/with-baseball-action.tssupabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql
💤 Files with no reviewable changes (38)
- .taskmaster/README.md
- .taskmaster/logs/.gitkeep
- .taskmaster/docs/prd.txt
- age-ratings-snapshot.yml
- .taskmaster/docs/current-state.md
- src/components/golf/travel/index.ts
- src/components/golf/travel/ExpenseSummary.tsx
- src/components/baseball/coach/ModeToggle.tsx
- .taskmaster/docs/feature-checklist.md
- src/components/lifting/soreness/SorenessComplianceBoard.tsx
- .taskmaster/config.json
- .taskmaster/tasks/tasks.json
- src/components/golf/travel/ExpenseList.tsx
- app-info-snapshot.yml
- src/components/layout/mode-toggle.tsx
- full-snapshot.yml
- src/app/baseball/(dashboard)/dashboard/stats/upload/error.tsx
- .full-stack-feature/state.json
- src/components/baseball/stats/StatsUploadClient.tsx
- src/app/baseball/(dashboard)/dashboard/stats/upload/loading.tsx
- src/components/golf/travel/ExpenseForm.tsx
- full-snapshot2.yml
- src/hooks/use-notifications.ts
- .taskmaster/state.json
- src/components/features/notification-center.tsx
- src/components/lifting/soreness/TeamSorenessHeatmap.tsx
- src/components/lifting/soreness/index.ts
- .taskmaster/templates/task-template.json
- src/lib/baseball/lifting/use-live-set-sync.ts
- src/components/baseball/stats/UploadHistory.tsx
- src/components/baseball/stats/index.ts
- .full-stack-feature/01-requirements.md
- src/components/golf/coachhelm/insights/InsightFiltersPanel.tsx
- src/components/golf/coachhelm/insights/InsightBulkActions.tsx
- src/components/golf/coachhelm/insights/InsightSearchBar.tsx
- src/components/golf/coachhelm/insights/InsightExportModal.tsx
- src/hooks/golf/use-service-worker.ts
- src/lib/baseball/read-models/lift-programs.ts
Comment on lines
+555
to
+595
| bb379_pit_agg AS ( | ||
| -- Mirrors recalc's pitching SELECT (breadth migration lines 193-220) exactly, | ||
| -- including deriving w/l/sv/holds/blown_saves from `result` (always NULL on | ||
| -- our backfilled rows — no legacy source — so these are always 0 here). | ||
| SELECT | ||
| t.player_id, t.team_id, t.season_year, | ||
| COUNT(DISTINCT bsp.game_id)::integer AS g_p, | ||
| COUNT(CASE WHEN bsp.result = 'W' THEN 1 END)::integer AS w, | ||
| COUNT(CASE WHEN bsp.result = 'L' THEN 1 END)::integer AS l, | ||
| COUNT(CASE WHEN bsp.result = 'S' THEN 1 END)::integer AS sv, | ||
| COALESCE(SUM(bsp.ip), 0) AS ip, | ||
| COALESCE(SUM(bsp.h), 0)::integer AS h_allowed, | ||
| COALESCE(SUM(bsp.r), 0)::integer AS r_allowed, | ||
| COALESCE(SUM(bsp.er), 0)::integer AS er, | ||
| COALESCE(SUM(bsp.bb), 0)::integer AS bb_allowed, | ||
| COALESCE(SUM(bsp.k), 0)::integer AS k_thrown, | ||
| COALESCE(SUM(bsp.hr), 0)::integer AS hr_allowed, | ||
| COALESCE(SUM(bsp.gf), 0)::integer AS gf, | ||
| (COUNT(CASE WHEN bsp.result = 'H' THEN 1 END)::integer + COALESCE(SUM(bsp.holds), 0)::integer) AS holds, | ||
| (COUNT(CASE WHEN bsp.result = 'BS' THEN 1 END)::integer + COALESCE(SUM(bsp.blown_saves), 0)::integer) AS blown_saves, | ||
| COALESCE(SUM(bsp.bf), 0)::integer AS bf, | ||
| COALESCE(SUM(bsp.hbp), 0)::integer AS p_hbp, | ||
| COALESCE(SUM(bsp.wp), 0)::integer AS wp | ||
| FROM bb379_touched t | ||
| JOIN public.baseball_box_score_pitching bsp | ||
| ON bsp.player_id = t.player_id AND bsp.team_id = t.team_id | ||
| JOIN public.baseball_games bg | ||
| ON bg.id = bsp.game_id AND bg.status = 'completed' | ||
| AND EXTRACT(YEAR FROM bg.game_date)::integer = t.season_year | ||
| GROUP BY t.player_id, t.team_id, t.season_year | ||
| ), | ||
| bb379_pit_final AS ( | ||
| -- era/whip/k9/bb9 mirror recalc's pitching rates (breadth migration lines | ||
| -- 222-227) exactly — division by raw `ip`, not outs-converted. | ||
| SELECT | ||
| p.*, | ||
| CASE WHEN p.ip > 0 THEN ROUND(9.0 * p.er / p.ip, 2) END AS era, | ||
| CASE WHEN p.ip > 0 THEN ROUND((p.bb_allowed + p.h_allowed)::numeric / p.ip, 3) END AS whip, | ||
| CASE WHEN p.ip > 0 THEN ROUND(9.0 * p.k_thrown / p.ip, 2) END AS k9, | ||
| CASE WHEN p.ip > 0 THEN ROUND(9.0 * p.bb_allowed / p.ip, 2) END AS bb9 | ||
| FROM bb379_pit_agg p |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the migration, recalc function, and memory registry/docs that may be impacted.
git ls-files 'supabase/migrations/*.sql' 'memory/registry.yml' 'memory/features/*.md' | sed -n '1,200p'
echo
echo '--- registry matches ---'
rg -n --no-heading 'baseball|legacy_stats|recalculate_baseball_season_stats' memory/registry.yml memory/features/*.md supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sqlRepository: njrini99-code/helmv3
Length of output: 26634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant sections of the migration and any referenced recalc function.
for f in supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql; do
echo "### $f"
wc -l "$f"
echo
# show around the cited lines
sed -n '520,640p' "$f"
echo
done
# Find the recalc function definition if present.
rg -n --no-heading 'recalculate_baseball_season_stats|SUM\(bsp\.ip\)|ROUND\(9\.0 \* p\.er / p\.ip' supabase/migrationsRepository: njrini99-code/helmv3
Length of output: 10631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the migration uses decimal innings elsewhere and whether the data model stores innings as outs or decimal notation.
rg -n --no-heading '\bip\b|innings|outs|decimal' supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql supabase/migrations | sed -n '1,200p'Repository: njrini99-code/helmv3
Length of output: 14657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the underlying table definition / comments describing baseball_box_score_pitching.ip.
rg -n --no-heading 'create table public\.baseball_box_score_pitching|baseball_box_score_pitching|ip\b' supabase/migrationsRepository: njrini99-code/helmv3
Length of output: 42498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## file list"
git ls-files 'supabase/migrations/*.sql' 'memory/registry.yml' 'memory/features/*.md' | sed -n '1,80p'
echo
echo "## relevant lines from migration"
sed -n '520,640p' supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql
echo
echo "## search for recalc and pitching ip semantics"
rg -n --no-heading 'recalculate_baseball_season_stats|SUM\(bsp\.ip\)|ROUND\(9\.0 \* p\.er / p\.ip|baseball_box_score_pitching|CREATE TABLE public\.baseball_box_score_pitching|ip\b' supabase/migrations memory/registry.yml memory/features/*.mdRepository: njrini99-code/helmv3
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## registry entries mentioning baseball"
sed -n '1,220p' memory/registry.yml
echo
echo "## feature docs mentioning baseball"
for f in memory/features/*.md; do
if rg -q 'baseball|box score|season stats|recalculate_baseball_season_stats' "$f"; then
echo "### $f"
rg -n 'baseball|box score|season stats|recalculate_baseball_season_stats' "$f"
echo
fi
done
echo
echo "## migration snippets around ip usage"
rg -n -A3 -B3 'SUM\(bsp\.ip\)|ROUND\(9\.0 \* p\.er / p\.ip|baseball_box_score_pitching' supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sqlRepository: njrini99-code/helmv3
Length of output: 11846
Aggregate pitching innings as outs before computing season stats. supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql:565-594 stores pitching ip in X.1/X.2 notation, so SUM(bsp.ip) turns 6.2 + 4.1 into 10.3 instead of 11 innings. That carries wrong values into ip, ERA, WHIP, K/9, and BB/9 in recalculate_baseball_season_stats() and this backfill; normalize to outs before aggregating.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql` around
lines 555 - 595, The bb379_pit_agg aggregation currently sums X.1/X.2 notation
as decimal innings, producing incorrect totals and rate stats. Normalize bsp.ip
to total outs before aggregation, sum the outs, and convert the result back to
baseball innings notation for ip; ensure bb379_pit_final’s ERA, WHIP, K/9, and
BB/9 calculations use the normalized value consistently.
…ot in public The 42883 failure reproduced on the CI fresh-stack replay and would have occurred identically on prod at apply time: pgcrypto lives in the extensions schema in both environments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
coderabbitai
Bot
added
ci
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/app/baseball/join/page.tsx (1)
34-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce alphanumeric validation to match the hint text and protect the route push.
The helper text states the invite code should contain "letters and numbers", but the submission handler only checks for length. Adding an alphanumeric regex check ensures users receive clear feedback if they type invalid characters and prevents special URI characters (like
?,#, or/) from breaking the subsequentrouter.pushpath.🐛 Proposed fix
const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); setError(null); if (!trimmed) { setError('Please enter an invite code.'); return; } if (trimmed.length < 4) { setError('Invite code must be at least 4 characters.'); return; } + + if (!/^[A-Z0-9]+$/.test(trimmed)) { + setError('Invite code must contain only letters and numbers.'); + return; + } router.push(`/baseball/join/${trimmed}`); };🤖 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/join/page.tsx` around lines 34 - 49, Update handleSubmit to validate trimmed against an alphanumeric-only pattern after the existing length check and before router.push. Set a clear invite-code error and return when any non-letter or non-number character is present, preserving the existing behavior for empty, short, and valid codes.src/components/baseball/import-center/ImportWizardClient.tsx (1)
1743-1745: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
src/components/baseball/import-center/ImportWizardClient.tsx:1744
Fix hydration mismatch risk on date formatting.Formatting a date with
toLocaleDateString()during SSR causes hydration mismatches when the server and client are in different time zones. AddsuppressHydrationWarningto the enclosing element so React ignores the text discrepancy during hydration.🐛 Proposed fix
- <td className="px-4 py-2 text-text-secondary"> + <td className="px-4 py-2 text-text-secondary" suppressHydrationWarning> {u.created_at ? new Date(u.created_at).toLocaleDateString() : '—'} </td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/baseball/import-center/ImportWizardClient.tsx` around lines 1743 - 1745, Add suppressHydrationWarning to the enclosing td element rendering u.created_at in the import wizard table, leaving the existing conditional date formatting unchanged.Source: Linters/SAST tools
🤖 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 @.github/workflows/visual-audit.yml:
- Around line 30-40: Restrict the workflow_dispatch base_url used by the visual
audit before any credentials are submitted. Update the base_url input and the
crawl setup around the Playwright configuration to either remove URL
configurability or validate that it is HTTPS and matches an explicit allowlist
of approved production hostnames, rejecting all other origins.
In `@docs/baseball/legacy-backfill-runbook.md`:
- Around line 304-310: Update the digest call in the hashed CTE to use the
established extensions.digest function instead of public.digest, matching the
schema created by the migration and allowing rollback SQL to execute.
In `@e2e/visual-audit.spec.ts`:
- Around line 285-299: Update the route-discovery flow around
captureRouteAllViewports so it collects visible navigation links from both
audited viewports, including mobile bottom navigation and drawer destinations,
rather than discovering only from the final desktop page state. Return or expose
the links found during each viewport capture, merge them into frontier with the
existing visited/frontier deduplication, and preserve the navigation priority
convention for primary versus secondary destinations.
In `@src/lib/baseball/__tests__/engine-event-derived.test.ts`:
- Around line 199-224: Migrate the bespoke Supabase fakes to the shared test
client from src/test/ in all three affected files: replace makeClient in
src/lib/baseball/__tests__/engine-event-derived.test.ts (199-224), replace the
manual filtering and thenable fake in
src/lib/baseball/__tests__/outcome-sweep-event-velocity.test.ts (24-79), and
migrate the containing fake in
src/lib/baseball/__tests__/outcome-sweep-insight-resolve.test.ts (61-66) so the
compatibility-only is stub is removed. Preserve each test’s seeded data and
configured read failures while relying on the shared client’s production-like
query semantics, including team scoping.
In `@src/lib/baseball/coachhelm/action-baseline.ts`:
- Around line 230-235: Update the metric-loading flow around loadEngineEventRows
and loadPlayerMetrics so event history is fetched only when targetMetric belongs
to the velocity metric set. For non-velocity metrics, skip loadEngineEventRows,
set eventDerived to null, and continue passing it to loadPlayerMetrics.
In `@src/lib/baseball/coachhelm/engine-event-derived.ts`:
- Around line 71-74: Replace the any-typed from contract in
EngineEventRowsClient with the typed SupabaseClient<Database>['from'] shape or a
shared Pick<SupabaseClient<Database>, 'from'> alias. Remove the eslint
suppression and preserve type checking across the chained query methods for both
RLS and admin clients.
In `@src/lib/baseball/coachhelm/outcome-sweep.ts`:
- Around line 205-220: Update the outcome-sweep event processing around
eventDerivedVelocityForPlayer to index eventRows.pitches by pitcher/player ID
and eventRows.battedBalls by batter/player ID once before iterating actions. For
each action, retrieve only that player’s rows from the maps, then apply the
existing createdAt measured_at > createdAt window before calling
eventDerivedVelocityForPlayer, preserving the current no-createdAt behavior.
---
Duplicate comments:
In `@src/app/baseball/join/page.tsx`:
- Around line 34-49: Update handleSubmit to validate trimmed against an
alphanumeric-only pattern after the existing length check and before
router.push. Set a clear invite-code error and return when any non-letter or
non-number character is present, preserving the existing behavior for empty,
short, and valid codes.
In `@src/components/baseball/import-center/ImportWizardClient.tsx`:
- Around line 1743-1745: Add suppressHydrationWarning to the enclosing td
element rendering u.created_at in the import wizard table, leaving the existing
conditional date formatting unchanged.
🪄 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: 55ce7eae-5271-4548-a580-1c4f51a32ed9
⛔ Files ignored due to path filters (2)
.full-stack-feature/01-requirements.mdis excluded by!.full-stack-feature*/**.full-stack-feature/state.jsonis excluded by!.full-stack-feature*/**
📒 Files selected for processing (76)
.github/workflows/visual-audit.yml.taskmaster/README.md.taskmaster/config.json.taskmaster/docs/current-state.md.taskmaster/docs/feature-checklist.md.taskmaster/docs/prd.txt.taskmaster/logs/.gitkeep.taskmaster/state.json.taskmaster/tasks/tasks.json.taskmaster/templates/task-template.jsonage-ratings-snapshot.ymlapp-info-snapshot.ymldocs/baseball/legacy-backfill-runbook.mde2e/baseball-stats-smoke.spec.tse2e/visual-audit.spec.tsfull-snapshot.ymlfull-snapshot2.ymlplaywright.config.tssrc/app/baseball/(dashboard)/dashboard/import/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats-center/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/upload/error.tsxsrc/app/baseball/(dashboard)/dashboard/stats/upload/loading.tsxsrc/app/baseball/(dashboard)/dashboard/stats/upload/page.tsxsrc/app/baseball/(public)/PublicMotionScope.test.tsxsrc/app/baseball/(public)/PublicMotionScope.tsxsrc/app/baseball/(public)/layout.tsxsrc/app/baseball/__tests__/page.test.tsxsrc/app/baseball/actions/__tests__/imports-capability-shape-gate.test.tssrc/app/baseball/actions/imports.tssrc/app/baseball/join/page.tsxsrc/app/baseball/page.tsxsrc/components/baseball/coach/ModeToggle.tsxsrc/components/baseball/command-center/CommandCenterFairway.tsxsrc/components/baseball/import-center/ImportCenterShell.tsxsrc/components/baseball/import-center/ImportWizardClient.tsxsrc/components/baseball/import-center/__tests__/ImportWizardClient.quick-box-score.test.tsxsrc/components/baseball/marketing/BaseballMarketingMotionScope.tsxsrc/components/baseball/stats-center/StatsCenterClient.tsxsrc/components/baseball/stats/StatsUploadClient.tsxsrc/components/baseball/stats/UploadHistory.tsxsrc/components/baseball/stats/index.tssrc/components/features/notification-center.tsxsrc/components/golf/coachhelm/insights/InsightBulkActions.tsxsrc/components/golf/coachhelm/insights/InsightExportModal.tsxsrc/components/golf/coachhelm/insights/InsightFiltersPanel.tsxsrc/components/golf/coachhelm/insights/InsightSearchBar.tsxsrc/components/golf/travel/ExpenseForm.tsxsrc/components/golf/travel/ExpenseList.tsxsrc/components/golf/travel/ExpenseSummary.tsxsrc/components/golf/travel/index.tssrc/components/landing/Footer.tsxsrc/components/layout/mode-toggle.tsxsrc/components/lifting/soreness/SorenessComplianceBoard.tsxsrc/components/lifting/soreness/TeamSorenessHeatmap.tsxsrc/components/lifting/soreness/index.tssrc/hooks/golf/use-service-worker.tssrc/hooks/use-notifications.tssrc/lib/baseball/__tests__/action-baseline-event-velocity.test.tssrc/lib/baseball/__tests__/action-baseline.test.tssrc/lib/baseball/__tests__/engine-event-derived.test.tssrc/lib/baseball/__tests__/engine-run-event-velocity.test.tssrc/lib/baseball/__tests__/outcome-sweep-event-velocity.test.tssrc/lib/baseball/__tests__/outcome-sweep-insight-resolve.test.tssrc/lib/baseball/__tests__/settings-aliases-and-legacy-redirects.test.tssrc/lib/baseball/coachhelm/action-baseline.tssrc/lib/baseball/coachhelm/engine-event-derived.tssrc/lib/baseball/coachhelm/engine-run.tssrc/lib/baseball/coachhelm/outcome-sweep.tssrc/lib/baseball/import-roster.tssrc/lib/baseball/lifting/use-live-set-sync.tssrc/lib/baseball/read-models/__tests__/elite-stat-events.test.tssrc/lib/baseball/read-models/elite-stat-events.tssrc/lib/baseball/read-models/lift-programs.tssrc/lib/baseball/stat-layer-manifest.tssrc/lib/baseball/with-baseball-action.tssupabase/migrations/20260715141727_baseball_legacy_stats_backfill.sql
💤 Files with no reviewable changes (36)
- .taskmaster/config.json
- .taskmaster/README.md
- src/components/golf/travel/ExpenseSummary.tsx
- .taskmaster/templates/task-template.json
- full-snapshot.yml
- .taskmaster/tasks/tasks.json
- .taskmaster/docs/feature-checklist.md
- age-ratings-snapshot.yml
- src/components/features/notification-center.tsx
- src/app/baseball/(dashboard)/dashboard/stats/upload/loading.tsx
- .taskmaster/state.json
- src/components/lifting/soreness/index.ts
- full-snapshot2.yml
- src/components/lifting/soreness/TeamSorenessHeatmap.tsx
- src/components/golf/coachhelm/insights/InsightBulkActions.tsx
- src/components/golf/travel/index.ts
- app-info-snapshot.yml
- .taskmaster/docs/current-state.md
- src/components/layout/mode-toggle.tsx
- src/components/baseball/stats/UploadHistory.tsx
- src/components/golf/travel/ExpenseList.tsx
- src/components/golf/travel/ExpenseForm.tsx
- .taskmaster/logs/.gitkeep
- src/components/golf/coachhelm/insights/InsightSearchBar.tsx
- src/components/golf/coachhelm/insights/InsightExportModal.tsx
- src/components/baseball/stats/index.ts
- src/hooks/use-notifications.ts
- src/lib/baseball/lifting/use-live-set-sync.ts
- .taskmaster/docs/prd.txt
- src/app/baseball/(dashboard)/dashboard/stats/upload/error.tsx
- src/components/lifting/soreness/SorenessComplianceBoard.tsx
- src/components/golf/coachhelm/insights/InsightFiltersPanel.tsx
- src/lib/baseball/read-models/lift-programs.ts
- src/components/baseball/coach/ModeToggle.tsx
- src/hooks/golf/use-service-worker.ts
- src/components/baseball/stats/StatsUploadClient.tsx
Comment on lines
+30
to
+40
| # SECURITY: consumes ZERO untrusted user input — the only `${{ }}` expressions | ||
| # are a trusted workflow_dispatch string input, secrets, and github.run_number. | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| base_url: | ||
| description: Base URL to crawl (playwright.config.ts baseURL) | ||
| required: false | ||
| default: "https://helmsportslabs.com" | ||
| type: string |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Block arbitrary audit origins before sending production credentials.
.github/workflows/visual-audit.yml:55 accepts any dispatch URL, then Playwright submits the coach/player secrets to that origin’s /baseball/login. A dispatcher can therefore direct production credentials to an external host. Remove configurability or validate an HTTPS hostname allowlist before the crawl.
Proposed allowlist guard
jobs:
visual-audit:
+ environment: production
env:
PLAYWRIGHT_BASE_URL: ${{ inputs.base_url }}
@@
steps:
+ - name: Validate audit target
+ env:
+ AUDIT_URL: ${{ inputs.base_url }}
+ run: |
+ node -e '
+ const url = new URL(process.env.AUDIT_URL);
+ const allowed = new Set(["helmsportslabs.com"]);
+ if (url.protocol !== "https:" || !allowed.has(url.hostname)) {
+ throw new Error(`Unapproved visual-audit target: ${url.origin}`);
+ }
+ 'Also applies to: 54-63
🤖 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 @.github/workflows/visual-audit.yml around lines 30 - 40, Restrict the
workflow_dispatch base_url used by the visual audit before any credentials are
submitted. Update the base_url input and the crawl setup around the Playwright
configuration to either remove URL configurability or validate that it is HTTPS
and matches an explicit allowlist of approved production hostnames, rejecting
all other origins.
Comment on lines
+285
to
+299
| for (let i = 0; i < frontier.length; i++) { | ||
| const route = frontier[i]; | ||
| if (!route || visited.has(route)) continue; | ||
| visited.add(route); | ||
|
|
||
| const capture = await captureRouteAllViewports(page, route, captures.length + 1, role, true); | ||
| captures.push(capture); | ||
|
|
||
| const navigatedOk = VIEWPORTS.some((vp) => capture.files[vp.name]); | ||
| if (!navigatedOk) continue; // this one route failed — data-capture only, keep crawling. | ||
|
|
||
| for (const nested of await discoverVisibleNavLinks(page)) { | ||
| if (!visited.has(nested) && !frontier.includes(nested)) frontier.push(nested); | ||
| } | ||
| for (const link of await discoverPublicSampleLinks(page)) publicSamples.add(link); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Discover navigation at both audited viewports.
e2e/visual-audit.spec.ts:296 discovers nested routes only after captureRouteAllViewports leaves the page at desktop width. Mobile-only bottom-nav or drawer destinations are therefore omitted from the phone audit. Return discovered links from each viewport capture and merge both sets into frontier.
As per path instructions, “Reserve bottom navigation for primary everyday destinations and the side drawer for secondary, team, admin, or account destinations.”
🤖 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/visual-audit.spec.ts` around lines 285 - 299, Update the route-discovery
flow around captureRouteAllViewports so it collects visible navigation links
from both audited viewports, including mobile bottom navigation and drawer
destinations, rather than discovering only from the final desktop page state.
Return or expose the links found during each viewport capture, merge them into
frontier with the existing visited/frontier deduplication, and preserve the
navigation priority convention for primary versus secondary destinations.
Source: Path instructions
Comment on lines
+199
to
+224
| function makeClient(tables: Record<string, Row[]>, errorTables: Set<string> = new Set()) { | ||
| return { | ||
| from(table: string) { | ||
| let rows = tables[table] ?? []; | ||
| const fail = errorTables.has(table); | ||
| const builder: Record<string, unknown> = { | ||
| select: () => builder, | ||
| eq: () => builder, | ||
| is: (col: string, val: unknown) => { | ||
| rows = rows.filter((r) => r[col] === val); | ||
| return builder; | ||
| }, | ||
| in: (col: string, vals: unknown[]) => { | ||
| rows = rows.filter((r) => vals.includes(r[col])); | ||
| return builder; | ||
| }, | ||
| order: () => builder, | ||
| range: () => | ||
| Promise.resolve( | ||
| fail ? { data: null, error: { message: `${table} read failed` } } : { data: rows, error: null }, | ||
| ), | ||
| }; | ||
| return builder; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Migrate the event-velocity tests to the shared Supabase test client.
The three bespoke query builders already differ from production semantics and require manual method stubs whenever query chains evolve.
src/lib/baseball/__tests__/engine-event-derived.test.ts#L199-L224: replacemakeClient; its no-op.eq()does not test team scoping.src/lib/baseball/__tests__/outcome-sweep-event-velocity.test.ts#L24-L79: replace the manually implemented filtering and thenable behavior.src/lib/baseball/__tests__/outcome-sweep-insight-resolve.test.ts#L61-L66: remove the compatibility-only.is()stub by migrating the containing fake.
As per path instructions, “Mock Supabase via the test client in src/test/.”
📍 Affects 3 files
src/lib/baseball/__tests__/engine-event-derived.test.ts#L199-L224(this comment)src/lib/baseball/__tests__/outcome-sweep-event-velocity.test.ts#L24-L79src/lib/baseball/__tests__/outcome-sweep-insight-resolve.test.ts#L61-L66
🤖 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__/engine-event-derived.test.ts` around lines 199 -
224, Migrate the bespoke Supabase fakes to the shared test client from src/test/
in all three affected files: replace makeClient in
src/lib/baseball/__tests__/engine-event-derived.test.ts (199-224), replace the
manual filtering and thenable fake in
src/lib/baseball/__tests__/outcome-sweep-event-velocity.test.ts (24-79), and
migrate the containing fake in
src/lib/baseball/__tests__/outcome-sweep-insight-resolve.test.ts (61-66) so the
compatibility-only is stub is removed. Preserve each test’s seeded data and
configured read failures while relying on the shared client’s production-like
query semantics, including team scoping.
Source: Path instructions
Comment on lines
+230
to
+235
| const { data: eventRows } = await loadEngineEventRows(supabase, teamId, [playerId]); | ||
| const eventDerived = eventRows | ||
| ? eventDerivedVelocityForPlayer(playerId, eventRows.pitches, eventRows.battedBalls) | ||
| : null; | ||
|
|
||
| const loaded = loadPlayerMetrics(playerId, statRows ?? [], undefined, eventDerived); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid loading full event history for non-velocity metrics.
src/lib/baseball/coachhelm/action-baseline.ts:230-235 performs two paginated event-table reads even when targetMetric is unrelated to velocity. Gate this call behind the velocity metric set and pass null otherwise.
🤖 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/coachhelm/action-baseline.ts` around lines 230 - 235, Update
the metric-loading flow around loadEngineEventRows and loadPlayerMetrics so
event history is fetched only when targetMetric belongs to the velocity metric
set. For non-velocity metrics, skip loadEngineEventRows, set eventDerived to
null, and continue passing it to loadPlayerMetrics.
Comment on lines
+71
to
+74
| export type EngineEventRowsClient = { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| from: (table: string) => any; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## registry\n'
git ls-files memory/registry.yml memory/features | sed -n '1,40p'
printf '\n## feature mapping\n'
sed -n '1,220p' memory/registry.yml
printf '\n## feature docs\n'
for f in $(git ls-files 'memory/features/*.md'); do
echo "\n### $f"
sed -n '1,220p' "$f"
done
printf '\n## target file outline\n'
ast-grep outline src/lib/baseball/coachhelm/engine-event-derived.ts --view expanded
printf '\n## target file contents\n'
cat -n src/lib/baseball/coachhelm/engine-event-derived.ts | sed -n '1,220p'
printf '\n## supabase client definitions\n'
ast-grep outline src/lib/supabase/client.ts --view expanded
printf '\n--- client.ts ---\n'
cat -n src/lib/supabase/client.ts | sed -n '1,220p'
printf '\n## server supabase definitions\n'
ast-grep outline src/lib/supabase/server.ts --view expanded
printf '\n--- server.ts ---\n'
cat -n src/lib/supabase/server.ts | sed -n '1,220p'
printf '\n## any related types\n'
rg -n "EngineEventRowsClient|createClient\(|from: \(table: string\) => any|supabase client contract|typed Supabase" src memory -SRepository: njrini99-code/helmv3
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## target file (relevant slice)\n'
cat -n src/lib/baseball/coachhelm/engine-event-derived.ts | sed -n '1,220p'
printf '\n## client/server supabase files\n'
for f in src/lib/supabase/client.ts src/lib/supabase/server.ts src/lib/supabase/admin.ts src/lib/supabase/admin-client.ts; do
if [ -f "$f" ]; then
echo "\n### $f"
cat -n "$f" | sed -n '1,220p'
fi
done
printf '\n## search for EngineEventRowsClient and from signatures\n'
rg -n "EngineEventRowsClient|from: \\(table: string\\) => any|createClient\\(" src/lib/supabase src/lib/baseball -S
printf '\n## exact surrounding lines in target file\n'
sed -n '60,90p' src/lib/baseball/coachhelm/engine-event-derived.ts | cat -nRepository: njrini99-code/helmv3
Length of output: 23573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## shared db/client aliases\n'
rg -n "type Db =|interface Db|Pick<.*from|SupabaseClient<|createClient\\(\\) as Db|from: \\(table: string\\) => any" src/lib/baseball src/lib/supabase -S
printf '\n## adjacent helpers with same pattern\n'
for f in \
src/lib/baseball/coachhelm/engine-stat-rows.ts \
src/lib/baseball/coachhelm/action-baseline.ts \
src/lib/baseball/coachhelm/engine-run.ts \
src/lib/baseball/coachhelm/outcome-sweep.ts \
src/lib/baseball/daily-contract/missed-sweep.ts \
src/lib/baseball/tasks/reminder-sweep.ts
do
if [ -f "$f" ]; then
echo "\n### $f"
sed -n '1,140p' "$f" | cat -n
fi
doneRepository: njrini99-code/helmv3
Length of output: 48969
Replace the any-typed client contract here. src/lib/baseball/coachhelm/engine-event-derived.ts:71-74 drops type checking for every from(...).select(...).eq(...).in(...).order(...).range(...) chain. Use the same SupabaseClient<Database>['from'] shape, or a shared Pick<SupabaseClient<Database>, 'from'> alias, so both the RLS and admin clients stay typed without the suppression.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/baseball/coachhelm/engine-event-derived.ts` around lines 71 - 74,
Replace the any-typed from contract in EngineEventRowsClient with the typed
SupabaseClient<Database>['from'] shape or a shared
Pick<SupabaseClient<Database>, 'from'> alias. Remove the eslint suppression and
preserve type checking across the chained query methods for both RLS and admin
clients.
Source: Coding guidelines
Comment on lines
+205
to
+220
| // Event rows get the SAME after-window filter (measured_at strictly after | ||
| // created_at) so an event-derived velocity metric is apples-to-apples with | ||
| // the box-score after-window above -- a pre-action pitch/batted-ball must | ||
| // never count toward "did it move" measurement. | ||
| const afterPitches = eventRows | ||
| ? createdAt | ||
| ? eventRows.pitches.filter((p) => !!p.measured_at && p.measured_at > createdAt) | ||
| : eventRows.pitches | ||
| : []; | ||
| const afterBattedBalls = eventRows | ||
| ? createdAt | ||
| ? eventRows.battedBalls.filter((b) => !!b.measured_at && b.measured_at > createdAt) | ||
| : eventRows.battedBalls | ||
| : []; | ||
| const eventDerivedForPlayer = eventRows | ||
| ? eventDerivedVelocityForPlayer(a.player_id!, afterPitches, afterBattedBalls) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Index event rows by player before processing actions.
src/lib/baseball/coachhelm/outcome-sweep.ts:209-220 scans every scoped pitch and batted-ball row twice per action, then filters by player again in eventDerivedVelocityForPlayer. With full-history reads, this becomes O(actions × events). Build pitcher/batter maps once and apply each action’s date window only to that player’s rows.
🤖 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/coachhelm/outcome-sweep.ts` around lines 205 - 220, Update
the outcome-sweep event processing around eventDerivedVelocityForPlayer to index
eventRows.pitches by pitcher/player ID and eventRows.battedBalls by
batter/player ID once before iterating actions. For each action, retrieve only
that player’s rows from the maps, then apply the existing createdAt measured_at
> createdAt window before calling eventDerivedVelocityForPlayer, preserving the
current no-createdAt behavior.
Owner
Author
|
🤖 Mission Control — PR summary What it changes (78 files, +4,636 / −10,881 — net deletion from the de-vibe pass). A batch integration branch bundling six workstreams:
Risk / areas to watch
CI — Posted by the automated Mission Control sweep. Not a review approval — CodeRabbit full review was requested separately. |
…kfill (CodeRabbit #868) - Copy-only summary now lists Step 4's baseball_player_season_stats write (finding 1). - Add permanent, service-role-only baseball_legacy_backfill_manifest ledger (RLS enabled, anon/authenticated revoked); every Step 1-4 INSERT records its own RETURNING rows into it, same transaction, tagged with a run_tag. Rollback now joins against the manifest instead of recomputing deterministic ids from current (possibly-changed) baseball_player_stats, and the runbook's rollback + season-stats-rollback sections are rewritten around manifest-join DELETEs. Verified recalculate_baseball_season_stats() does a full from-scratch rebuild (not an incremental merge) before writing the "safe to delete" rollback caveat (finding 2). - Take an explicit LOCK TABLE ... IN SHARE ROW EXCLUSIVE MODE on all 5 read/written tables before the eligibility snapshot; runbook gains an apply- window note. Confirmed SHARE ROW EXCLUSIVE cannot self-conflict with this migration's own later INSERTs (finding 9). - Rename the two TEMP TABLEs to the required baseball_ prefix, all references (finding 10). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
…h (CodeRabbit #868) The hint text promises "letters and numbers" but only length was checked, letting URI-breaking characters (?, #, /) reach router.push(`/baseball/join/${trimmed}`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
…ell (CodeRabbit #868) toLocaleDateString() formats with the server's locale/timezone during SSR but the browser's on hydration, risking a mismatch warning. Matches this repo's existing suppressHydrationWarning-on-the-enclosing-element precedent (LocalTime.tsx, RelativeTime.tsx, Fairway calendar/announcements components). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
…r capability (CodeRabbit #868) canManageImports=false conflated stats-capable staff (routed through the /stats/upload shim) with staff holding NEITHER can_manage_imports nor can_manage_stats, whom both routes would just bounce off their own middleware gate. page.tsx now Promise.all's a second hasBaseballCapability call for can_manage_stats and passes both down; StatsCenterClient renders the header "Import Center" action and the empty-state "Import a box score" CTA only when canManageImports || canManageStats holds, keeping the existing importEntryHref branch for the visible cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
…ze counts (CodeRabbit #868) avg_exit_velocity/avg_launch_angle (hitting) and avg_velocity (pitching) each correctly narrow sampleSize to rows with an actual non-null reading, but still passed the FULL bbProv/pProv array (every batted ball / pitch, hand-charted or radar-read) into dominantTrust/dominantContext. A majority of hand-charted, no-reading rows could drag trustTier down to 'unverified' even when every row that fed the average was 'official' radar data. Pass the same `.filter(reading != null)` array as provenance in all three call sites. Extends the #864 sampleSize-honesty suite with mixed-trust regression tests (few official radar rows + many unverified hand-charted rows -> trustTier must reflect only the radar rows) for the batting and pitching paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
…eject empty results (CodeRabbit #868) Two related fixes to withBaseballAction: - The (possibly args-conditional) requiredCapability resolver ran BEFORE Sentry.withScope/the wrapper's own try/catch even started, so a throwing resolver (e.g. a malformed/omitted argument) threw raw and unsanitized, skipping AUTH, Sentry, and logServerException entirely. Resolution now happens inside the guarded try/catch, right after AUTH resolves and before capability enforcement — a throwing resolver now produces the same sanitized BaseballActionError + Sentry-logged path as any other action failure. Still resolved exactly once, from the same args reference; tags/ breadcrumbs are set from the resolved value immediately afterward. - requiredCapability's array forms are now typed as non-empty tuples (readonly [BaseballCapability, ...BaseballCapability[]]) so `[]` is a compile-time error, and a resolver that manufactures an empty array at runtime anyway is rejected with a thrown BaseballCapabilityError (fail closed) instead of falling through to `resolvedCapabilityList[-1]` === undefined being passed to requireBaseballCapability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 8 minutes. |
This was referenced Jul 17, 2026
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Integration batch of 10 task PRs, each individually gated (typecheck / eslint / vitest / cycles) and adversarially reviewed before merging into this branch.
What's in it
/baseball/joincode-entry page. Signed-in redirect behavior preserved.(public)route group (team/player/program/packet) rendered Living Annualm-atoms with noLazyMotionancestor: names/stat numerals were frozen at opacity 0 on live public recruiting pages. SharedPublicMotionScopewrapper + real (unmocked) framer-motion regression test./stats/uploadis now a capability-aware shim (imports staff → full Import Center; stats-only staff → inline quick-box-score wizard).previewImport/commitImportgates are shape-scoped:game_box_scoreacceptscan_manage_statsORcan_manage_imports; every other shape stays imports-only. Upload-history surface restored.withBaseballAction.requiredCapabilitynow supports OR-lists and arg-resolvers (existing call sites byte-identical). New authorization test suite mocks only the capability seam — mutation-verified.workflow_dispatchGHA screenshot crawl (coach + player, 390/1440 viewports, live-DOM route discovery) against a deployed URL; spec gated behindVISUAL_AUDIT=1, excluded from all CI lanes.Review process
Every PR passed adversarial review; criticals found and fixed before merge included: capability lockouts across all three gate layers (page/middleware/action) in #863, an unbounded team-wide read and wrong sampleSize semantics in #864, and a season-stats clobber-risk misstatement in #862's runbook.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa