fix(notifications,ci): background-safe push dispatcher + unblock RLS required check — helm-review 2026-06-08 - #243
Conversation
…required check
helm-review 2026-06-08 — two production/CI fixes found from DB error_logs,
admin_events, and the live prod schema.
1) Push delivery crashed in background contexts with "`cookies` was called
outside a request scope" (logged via insight_notifier.send). Both
sendPushNotification() and getUserNotificationPreferences() used the
cookie-backed request client, which throws when called from the
post-write insight-notifier and the event-reminders cron (no request
scope). Switch the push dispatcher to the service-role admin client —
the same key the send-apns-push Edge Function already authenticates
with. This also fixes RLS silently hiding a *recipient's* device tokens
whenever the sender differs from the recipient (messages, announcements,
event RSVPs). getUserNotificationPreferences() gains an optional client
param (default unchanged) so email callers + existing tests are untouched.
2) supabase/tests/rls/golf_coach_insights_cross_tenant_select.sql seeded
insight_type='value_derived', which is NOT in
golf_coach_insights_insight_type_check (verified against the live
constraint). The bad INSERT aborts the pgTAP plan and reddens the
REQUIRED "Supabase lint + RLS tests" check on main HEAD — blocking every
PR. Swap to a valid type ('putting'). (Same fix PR #238 carries; included
here so this PR can go green off current origin/main.)
Adds src/test/lib/notifications/push.test.ts as a regression guard that the
push path never touches the cookie client.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
njrini99-code has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 29 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
![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: 51015acbf3
ℹ️ 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".
| // device tokens whenever the sender differs from the recipient. The | ||
| // send-apns-push Edge Function below already authenticates with this same | ||
| // service-role key. | ||
| const supabase = createAdminClient(); |
There was a problem hiding this comment.
Because this module is marked 'use server', the exported sendPushNotification is a remotely invokable Server Action, but it accepts an arbitrary userId without authenticating or authorizing the caller. Switching this query to the service-role client removes the previous RLS-based recipient restriction, so a caller who knows another user's ID can trigger crafted pushes to all of that user's devices. Keep the privileged dispatcher behind a non-action server-only boundary, or verify the caller and their permission to notify the recipient before creating the admin client.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Email path still uses cookie-backed client in cron contexts
The PR fixes the push notification path to use the admin client in background contexts, but sendEmailNotification at src/lib/notifications/email.ts:795 still calls getUserNotificationPreferences(recipientId) without passing a client, and getRecipientGreeting at email.ts:737 also calls await createClient() directly. The cron event-reminders route (src/app/api/cron/event-reminders/route.ts:251) calls sendBulkEmailNotification which flows through both of these, meaning emails from cron would hit the same "cookies outside request scope" error the PR fixes for push. The cron route catches the error (route.ts:251: .catch(() => {...})), so it's non-fatal, but emails silently fail. This is pre-existing and out of the PR's stated scope (push-only fix), but the infrastructure to fix it (the optional client parameter) is now in place and could be extended to the email path.
(Refers to line 795)
Was this helpful? React with 👍 or 👎 to provide feedback.
| it('honors the recipient opt-out (push_events=false) and sends nothing', async () => { | ||
| prefs = { ...DEFAULT_NOTIFICATION_PREFERENCES, push_events: false }; | ||
| const { sendPushNotification } = await import('@/lib/notifications/push'); | ||
| const result = await sendPushNotification('coachhelm_insight', 'user-1', { insightTitle: 'x' }); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(serverCreateClient).not.toHaveBeenCalled(); | ||
| expect(fetchSpy).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🔴 shouldSendPush routes coachhelm_insight to push_events instead of the dedicated push_coachhelm preference
The coachhelm_insight notification type is grouped with general events in shouldSendPush (src/lib/notifications/push.ts:21-23), so it checks prefs.push_events instead of the dedicated prefs.push_coachhelm. The push_coachhelm field exists in NotificationPreferences (src/lib/notifications/types.ts:41), has a default (types.ts:55), is validated in Zod (src/app/actions/notification-preferences.ts:23), and is exposed as its own toggle in the settings UI (settings/page.tsx:967: "CoachHelm Insights — Push when new insights or weekly digests land"). The email side correctly routes to its own email_coachhelm preference with an explicit comment about separating AI insights from team announcements (src/lib/notifications/email.ts:118-121). The consequence: toggling "CoachHelm Insights" push off in settings has zero effect — the user keeps receiving CoachHelm pushes as long as push_events is enabled. The new test at line 100 tests push_events: false for a coachhelm_insight notification, which passes only because of this routing bug and would need to be updated along with the fix.
Prompt for agents
The test at push.test.ts:99-107 tests coachhelm_insight opt-out using push_events: false, but the correct preference for CoachHelm push notifications is push_coachhelm (defined in types.ts:41, rendered in settings/page.tsx:967). Two things need to change together:
1. In src/lib/notifications/push.ts, function shouldSendPush: move the coachhelm_insight case out of the push_events group and give it its own case that returns prefs.push_coachhelm, mirroring how shouldSendEmail in email.ts:120-121 returns prefs.email_coachhelm.
2. In src/test/lib/notifications/push.test.ts:100, change the opt-out test to set push_coachhelm: false instead of push_events: false, so the test validates the intended user-facing behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing as stale-conflicting (June 8; notifications area has since been reworked). Branch preserved — rebase and reopen if the push-dispatcher hardening is still wanted. |
1 participant
helm-review — 2026-06-08
Daily review of Sentry-class errors (DB
error_logs+admin_events), Vercel prod runtime logs, and the live prod schema. Two real, in-scope fixes; everything else triaged as noise / known-deferred / already-tracked (full brief at the bottom).Fix 1 — Push delivery crashes in background contexts (
cookiesoutside request scope)Symptom (prod, logged):
insight-notifier: push send reported failure:+ "cookieswas called outside a request scope" (insight_notifier.send, 2026-06-08).Root cause:
sendPushNotification()(src/lib/notifications/push.ts) andgetUserNotificationPreferences()(src/lib/notifications/email.ts) both used the cookie-backed request client (@/lib/supabase/server→cookies()). They are invoked from background contexts with no request scope — the post-writeinsight-notifierand theevent-reminderscron — wherecookies()throws. The throw originates ingetUserNotificationPreferences(called before the device-token read), so the push silently fails.Secondary bug fixed in the same change: even in a request context the dispatcher reads the recipient's
device_tokensbyuser_id. With the cookie client + RLS, a user cannot see another user's tokens → cross-user pushes (new message, announcement, RSVP reminder) were silently returning "no tokens".Fix: the push dispatcher now uses the service-role admin client (
createAdminClient()) for prefs +device_tokens— the same service-role key thesend-apns-pushEdge Function already authenticates with.getUserNotificationPreferences()gains an optional client param (defaults to the cookie client, so email callers + the existingemail.test.tsare unchanged). No caller is a client component, so there is no new client-callable-server-action surface.Regression guard added:
src/test/lib/notifications/push.test.tsmocks the cookie client to throw and asserts the push path never touches it.Fix 2 — Unblock the RED required
Supabase lint + RLS testscheck onmainsupabase/tests/rls/golf_coach_insights_cross_tenant_select.sqlseededinsight_type='value_derived', which is not ingolf_coach_insights_insight_type_check(verified against the live constraint — valid set is 39 types incl.putting). The bad INSERT aborts the pgTAP plan and reddens the requiredSupabase lint + RLS testscheck onmainHEAD (6b23675e), which blocks every PR. Swapped to a valid type ('putting'). Same fix PR #238 carries; included here so this PR can go green off currentorigin/main.Verification (local, worktree off
origin/main)npx tsc --noEmit→ exit 0, 0 errorsnpx vitest run src/test/lib/notifications/→ all pass (incl. newpush.test.ts)npx eslinton changed files → cleanFiles
src/lib/notifications/push.ts— admin client for the dispatchersrc/lib/notifications/email.ts— optional client param (backward-compatible)src/test/lib/notifications/push.test.ts— new regression testsupabase/tests/rls/golf_coach_insights_cross_tenant_select.sql— valid seed typeDaily brief (full triage)
🔴 Highest-severity prod issue (NOT in this PR — needs human action):
golf_roundsis missing the table-wide UPDATE grant forauthenticated. Live prod has only 30 column-level UPDATE grants (incl. the 4 identity columns added by the already-applied conservative20260603040000), butstrokes_gained_*/coachhelm_*/ai_recap*fall outside it →42501 permission deniedon auto-save. 68 failures on 2026-06-05 + 15 on 06-06 (real round-data loss on/golf/dashboard/rounds/new). The durable fix (table-wide grant,20260607120000_..._tablewide.sql) is in PR #238 — open, all 4 required checks green when last run, awaiting human approval +supabase db pushto prod. The migration is not yet applied to the prod DB.🟢 "Rendered more hooks" / React #310 on
/golf/dashboard/stats— NOT a code bug. An exhaustive static audit (manual pass + a 9-agent workflow over 44 files covering the full Fairway and legacyStatsClientrender trees) found zero rules-of-hooks violations — every hook sits before its component's returns. The 15 dev occurrences co-occur with Turbopack HMR module errors (Fast-Refresh artifacts during active editing of the stats files); the single prod#310(06-06) aligns with stale-chunk deploy churn (same-day chunk-load failure + frequent CLI deploys). No fix made — fabricating one would be wrong.⚪ Noise / known-deferred (no action):
pattern-miner.thresholdsstarvation,philosophy gate filtered,causality unknown metric— routine CoachHelm cron telemetry (info/warning); causality drift is the known deferred registry-mapping item.[Stats] ... is9Hole is not defined— stale local dev-build chunk; symbol absent fromstats-data.tson origin/main + HEAD. Known false positive.Module [project]/src/app/golf/actions/data:...— Turbopack dev HMR module errors (localhost only).ci/circleci: lighthouse-previewred — chronic, non-required; do not block.Failed to register device token: RLS(1×) andLoading chunk 45682 failed(1×, prod) — single transient occurrences; watch.Vercel prod runtime logs (error+fatal, last 4d): clean (no matching entries).
🤖 Generated with Claude Code