Lift Lab unification → helm, CRM fixes, admin badge, e2e stabilization, Bridge incident fixes - #768
Merged
Merged
Conversation
…s statuses to CHECK domain demo_requests had only a public INSERT policy and a service_role ALL policy, so the admin Inbound Leads list (browser client, RLS) rendered 0 rows while the dashboard badge (server rollup, service role) counted 3. Adds admin-gated SELECT/UPDATE policies (applied to prod via MCP; file mirrored). The view also used status values the CHECK constraint rejects: filters expected 'new'/'converted' but the form inserts 'pending', and Add-to-CRM wrote 'converted' which demo_requests_status_check refuses — now mapped to the real domain (pending→New, completed→Added). Also stages the phase-3 graveyard migration (16 legacy lift/strength tables, apply AFTER the unification deploy). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KsHxAKSPhqgfJKTFfpjRiC
Both SECURITY DEFINER functions were EXECUTE-able by any authenticated user with no role check, exposing CRM calendar events and outreach analytics to coaches/players. Applied to prod via MCP; file mirrored for fresh databases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KsHxAKSPhqgfJKTFfpjRiC
…adge fix + e2e stabilization All 6 build lanes complete and self-verified (tsc/lint clean per lane): - lifting-v11.ts + group-audit-writer.ts fully helm-native; publishLiftDay writes helm directly (dual-write bridge deleted); dead createLiftExercise removed; audit → new helm_lifting_group_audit (applied to prod) - 10 read-model/loader files: residual legacy reads swapped; dead getStrengthGroupDetail removed - lifting.ts/program-settings.ts/lift-builder.ts + builder/live pages swapped - demo seed + coverage scripts helm-native - /admin/teams/[id] health badge: unfiltered teamLastActivity (36/36 tests) - e2e: CI demo-account lockout root-cause fixed (seed force-resets password + clears lockouts), fail-fast secrets step, login URL patterns, waitForPageLoad, 18 hard sleeps → web-first assertions, dead placeholder specs deleted - staged migrations: phase-3 graveyard (apply AFTER deploy), group-audit mirror Test-reconciliation + Bridge incident fixes land in follow-up commits on this branch. Gate before merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KsHxAKSPhqgfJKTFfpjRiC
…ation, revalidate split, feed noise, log-event hardening, dynamic bails + RLS tripwire Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KsHxAKSPhqgfJKTFfpjRiC
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Too many files changed for review. ( Bypass the limit by tagging |
|
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 ignored due to path filters (1)
📒 Files selected for processing (69)
Summary by CodeRabbit
WalkthroughThis PR migrates baseball lifting/strength data access from legacy ChangesHelm Lifting Lab Migration
Estimated code review effort: 4 (Complex) | ~75 minutes Playwright E2E Stabilization
Estimated code review effort: 3 (Moderate) | ~30 minutes Admin Telemetry Gating and Error Logging Hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LogRoute as "log-event/log-error route"
participant TelemetryGate as "telemetry-gate"
participant AdminEvents as "admin_events table"
Client->>LogRoute: POST body (text)
LogRoute->>LogRoute: check empty body
alt empty body
LogRoute-->>Client: 204
else invalid JSON
LogRoute-->>Client: 400 Invalid JSON
else valid JSON
LogRoute->>TelemetryGate: shouldPersistAdminTables()
TelemetryGate-->>LogRoute: true/false (CI/env check)
LogRoute->>TelemetryGate: getRuntimeEnv()
TelemetryGate-->>LogRoute: runtimeEnv tag
LogRoute->>AdminEvents: insert row with metadata.runtimeEnv
LogRoute-->>Client: 200 success
end
sequenceDiagram
participant Action as "lift-builder / lifting action"
participant HelmGroups as "helm_lifting_groups"
participant HelmMembers as "helm_lifting_group_members"
participant HelmAthletes as "helm_lifting_athletes"
participant HelmSessions as "helm_lifting_sessions"
Action->>HelmGroups: resolve organization_id/team_id for group
Action->>HelmMembers: fetch athlete_id for group
Action->>HelmAthletes: map athlete_id -> sport_player_id
Action->>HelmSessions: materialize/update sessions per athlete
HelmSessions-->>Action: session rows (legacy_baseball_id null)
Possibly related PRs
Suggested labels: ✨ 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 Comment |
coderabbitai
Bot
added
ci
PR Summary by QodoLift Lab helm unification, CRM RLS fixes, admin health badge, e2e stabilization
AI Description
Diagram
High-Level Assessment
Files changed (70)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d6c25d7e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+269
to
+271
| const { data: memberRows } = await fromUntyped(supabase, 'helm_lifting_group_members') | ||
| .select('athlete_id') | ||
| .in('group_id', groups) as { data: Array<{ athlete_id: string }> | null }; |
There was a problem hiding this comment.
After switching this path to helm_lifting_group_members, the action trusts any supplied groupScope and uses the returned athlete IDs directly. In an org with multiple baseball teams or cross-sport Lift Lab groups, a stale/forged group id can read another team/sport's group membership under the org-scoped helm RLS and then create sport: 'baseball' sessions under ctx.targetTeamId for those athletes. Validate the referenced helm_lifting_groups rows against organization_id, sport = 'baseball', and team_id = ctx.targetTeamId before materializing sessions.
Useful? React with 👍 / 👎.
Comment on lines
+315
to
+317
| const { data: memberRows } = await fromUntyped(supabase, 'helm_lifting_group_members') | ||
| .select('athlete_id') | ||
| .eq('group_id', input.groupId) as { data: Array<{ athlete_id: string }> | null }; |
There was a problem hiding this comment.
This builder path now resolves members from the cross-sport, org-scoped helm_lifting_group_members table without first proving that input.groupId is a baseball group for ctx.targetTeamId. If a request supplies another team’s group id in the same org, the code maps those athletes back to player ids and later creates sessions with the active team's team_id, assigning plans to players outside the active roster. Validate the group row's org/sport/team, or intersect the resolved players with the current team roster before creating sessions.
Useful? React with 👍 / 👎.
Code Review by Qodo
Context used✅ Compliance rules (platform):
97 rules 1. chain typed as any
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| function createAdminChain() { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const chain: any = {}; |
There was a problem hiding this comment.
1. chain typed as any 📘 Rule violation ⚙ Maintainability
The new test helper sets const chain: any = {}, introducing an explicit any type that bypasses
TypeScript safety. This violates the compliance rule that disallows any in changed TypeScript
code.
Agent Prompt
## Issue description
`const chain: any = {}` introduces an explicit `any`, which defeats TypeScript type-checking.
## Issue Context
This test file is newly added and can be typed without using `any` by defining a small interface/type for the mocked chain (including its `then` signature).
## Fix Focus Areas
- src/app/golf/actions/__tests__/crm-engagement.test.ts[40-53]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+86
to
+89
| const { data, error } = await (admin as any) | ||
| .from('crm_coach_engagement') | ||
| .select('coach_id, score, temperature, opens_90d, clicks_90d, last_event_at') | ||
| .in('coach_id', coachIds); |
There was a problem hiding this comment.
2. admin as any assertion 📘 Rule violation ⚙ Maintainability
getCoachEngagement casts admin to any via (admin as any), which bypasses type checking in a changed server action. This violates the compliance requirement to avoid any in TypeScript.
Agent Prompt
## Issue description
The code uses `(admin as any)` which introduces `any` via a type assertion, bypassing type safety.
## Issue Context
This file is a server action module; the query can be typed by giving `createAdminClient()` a concrete return type (or by creating a narrow interface for the subset used: `.from().select().in()`), avoiding `any`.
## Fix Focus Areas
- src/app/golf/actions/crm-engagement.ts[83-90]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+56
to
+60
| bucketed AS ( | ||
| SELECT CASE WHEN seconds_to_open < 60 THEN '0-60' WHEN seconds_to_open < 600 THEN '60-600' WHEN seconds_to_open < 3600 THEN '600-3600' WHEN seconds_to_open < 14400 THEN '3600-14400' WHEN seconds_to_open < 86400 THEN '14400-86400' ELSE '86400+' END AS bucket FROM paired | ||
| ) | ||
| SELECT (split_part(bucket, '-', 1))::int, CASE WHEN bucket = '86400+' THEN 999999 ELSE (split_part(bucket, '-', 2))::int END, COUNT(*)::int | ||
| FROM bucketed GROUP BY bucket ORDER BY 1; |
There was a problem hiding this comment.
4. Crm bucket cast crash 🐞 Bug ≡ Correctness
get_crm_time_to_open() can emit the bucket value 86400+ but still casts `split_part(bucket, '-', 1) to int`, which throws for that bucket and causes the RPC to fail at runtime. The CRM insights server action then logs the error and returns an empty dataset, silently breaking the time-to-open chart.
Agent Prompt
## Issue description
The SQL function `public.get_crm_time_to_open` constructs a bucket label `'86400+'` for opens >= 24h, but still casts `split_part(bucket, '-', 1)` to `int`. For `'86400+'`, `split_part(..., 1)` returns `'86400+'`, which is not castable to integer, causing the RPC to error.
## Issue Context
This RPC is called by the admin CRM insights page to render the time-to-open distribution; when it errors, the server action returns `[]`, silently hiding the chart data.
## Fix Focus Areas
- supabase/migrations/20260704110000_crm_rpcs_admin_gate.sql[56-60]
## Suggested fix
Change the SELECT to guard *both* `bucket_min` and `bucket_max` for the `'86400+'` bucket, e.g.:
- `bucket_min := CASE WHEN bucket='86400+' THEN 86400 ELSE split_part(bucket,'-',1)::int END`
- `bucket_max := CASE WHEN bucket='86400+' THEN 999999 ELSE split_part(bucket,'-',2)::int END`
Alternatively, avoid string buckets entirely by bucketing on numeric ranges and formatting labels in the app layer.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+30
to
+37
| // GitHub Actions sets both CI=true and GITHUB_ACTIONS=true on every job | ||
| // runner — never persist from there, independent of whatever VERCEL_ENV | ||
| // happens to read (it's normally unset in CI, but this must not depend | ||
| // on that staying true). | ||
| if (process.env.CI === 'true' || process.env.GITHUB_ACTIONS === 'true') return false; | ||
| // A VERCEL_ENV that exists but isn't 'production' (preview, or a local | ||
| // override) is excluded explicitly rather than falling through. | ||
| if (process.env.VERCEL_ENV && process.env.VERCEL_ENV !== 'production') return false; |
There was a problem hiding this comment.
5. Ci gate misses truthy 🐞 Bug ☼ Reliability
shouldPersistAdminTables()/getRuntimeEnv() only treat CI as CI when process.env.CI or process.env.GITHUB_ACTIONS equals the literal string 'true', so other common truthy values (e.g. '1') will fall through. In such environments, the telemetry gate may incorrectly allow persisting admin tables outside the intended production runtime.
Agent Prompt
## Issue description
The telemetry gate checks `process.env.CI === 'true'` and `process.env.GITHUB_ACTIONS === 'true'`. If a CI environment sets these variables to a non-`'true'` truthy value (commonly `'1'`), the gate won’t recognize CI and may allow persistence.
## Issue Context
This gate controls whether prod incident tables (`admin_events`, `error_logs`, etc.) are written at all.
## Fix Focus Areas
- src/lib/telemetry-gate.ts[27-56]
## Suggested fix
Treat these env vars as truthy if they are set and not explicitly false, e.g.:
- `const isCI = Boolean(process.env.GITHUB_ACTIONS) || (process.env.CI && process.env.CI !== 'false');`
Then reuse `isCI` in both `shouldPersistAdminTables()` and `getRuntimeEnv()` to keep the classification consistent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This was referenced Jul 3, 2026
7 tasks
This was referenced Jul 10, 2026
This was referenced Jul 17, 2026
4 tasks
This was referenced Aug 2, 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.
Lift Lab unification: program-builder → helm, CRM fixes, admin badge, e2e stabilization
One train, one deploy. Four workstreams:
1. Program-builder → helm_lifting_* (completes the Lift Lab schema split)
The last writers of the 16 legacy
baseball_lift_*/baseball_strength_*/ bodyweight / availability tables move tohelm_lifting_*(all legacy tables verified 0 rows in prod; 15/16 had exact helm mirrors, the 16th —helm_lifting_group_audit— was created + RLS'd in prod ahead of this PR):createLiftProgram/addLiftWeek/addLiftDay/addLiftSection/addLiftPrescription, update/delete/reorder/duplicate/template) now writes helm tables with the family renames (player_id→athlete_id, org+sport scoping,baseball_context→sport_context).publishLiftDayno longer materializes legacy rows and bridges — it writeshelm_lifting_program_assignments/sessions/session_exercisesdirectly. The legacy dual-space session resolver is deleted.program_id,week_number; daysweek_id,day_number; membersgroup_id,athlete_id; sessionsprogram_assignment_id,athlete_id; bodyweightathlete_id,entry_date).createLiftExercise+getStrengthGroupDetaildeleted.20260704090000, staged in this PR) moves all 16 legacy tables out ofpublic— every FK edge stays insidegraveyard(verified againstpg_constraint).2. CRM fixes (RLS already applied to prod; code here)
demo_requestshad no SELECT/UPDATE policy → the admin Inbound Leads list showed 0 while the dashboard badge counted 3 (a real Heidelberg lead sat invisible since June 11). Admin-gated policies applied + mirrored.new/converted) — now mapped to the real domain (pending/completed); Add-to-CRM no longer silently fails.get_crm_events_in_range+get_crm_time_to_openwere EXECUTE-able by any authenticated user → admin-gated (applied + mirrored).3. /admin/teams/[id] health badge
Detail page fed
classifyTeamHealthonly completed rounds by current roster members; /admin/golf uses unfiltered team activity. Detail now exposesteamLastActivity(unfiltered, one cheap query) for the badge; per-player roster columns unchanged.4. e2e stabilization (root causes from CI-log forensics — no local browser runs)
seed-baseball-demo.tsnever reset existing CI demo passwords, so secret drift + Playwright retries fed the 10-attempt DB lockout every run. Seed now force-sets the two synthetic CI identities' passwords and clears lockout state.playwright.yml(missing secret now fails in seconds, not after build+seed).auth.spec.ts,helpers/auth.ts), tolerantwaitForPageLoad, 18 hard sleeps → web-first assertions, golf specs migrated to env-gated fixtures, dead placeholder specs deleted.Verification
DB sequencing (for the record)
Applied to prod pre-merge (all additive/verified-safe with live code):
demo_requests_admin_read_update,crm_rpcs_admin_gate,helm_lifting_group_audit. Applied post-deploy:graveyard_legacy_liftlab_tables_phase3(16 tables, public 275→259, graveyard 16→32).🤖 Generated with Claude Code
https://claude.ai/code/session_01KsHxAKSPhqgfJKTFfpjRiC