fix(notifications,ci): background-safe push dispatcher + unblock RLS required check — helm-review 2026-06-08

src/lib/notifications/email.ts

@@ -5,7 +5,9 @@
  * Falls back gracefully if Resend is not configured.
  */
 
+import type { SupabaseClient } from '@supabase/supabase-js';
 import { createClient } from '@/lib/supabase/server';
+import type { Database } from '@/lib/types/database';
 import type { NotificationPreferences, NotificationType, EmailTemplate } from './types';
 import { DEFAULT_NOTIFICATION_PREFERENCES } from './types';
 
@@ -37,10 +39,15 @@ async function getResendClient() {
  * Falls back to defaults if preferences not set or column missing.
  */
 export async function getUserNotificationPreferences(
-  userId: string
+  userId: string,
+  client?: SupabaseClient<Database>
 ): Promise<NotificationPreferences> {
   try {
-    const supabase = await createClient();
+    // Accept a caller-supplied client so server-side dispatchers (e.g. the
+    // background insight-notifier / cron push paths) can hand in a service-role
+    // admin client. The cookie-backed request client throws "cookies was called
+    // outside a request scope" when invoked with no active request.
+    const supabase = client ?? (await createClient());
     // .maybeSingle() returns null (no error) when the user row does not exist.
     // The cron roster sweep can hand us stale or detached player_ids whose
     // backing `users` row was deleted; .single() would throw PGRST116

src/lib/notifications/push.ts

@@ -1,6 +1,6 @@
 'use server';
 
-import { createClient } from '@/lib/supabase/server';
+import { createAdminClient } from '@/lib/supabase/admin';
 import type { NotificationType, NotificationPreferences } from './types';
 import { getUserNotificationPreferences } from './email';
 
@@ -125,14 +125,22 @@ export async function sendPushNotification(
   data: Record<string, unknown>
 ): Promise<{ success: boolean; error?: string }> {
   try {
-    // Check user preferences
-    const prefs = await getUserNotificationPreferences(userId);
+    // Server-side dispatcher: use the service-role admin client for ALL DB
+    // access here. This works in BOTH request and background contexts (cron,
+    // roster-sweep, the post-write insight-notifier) — the cookie-backed
+    // request client throws "cookies was called outside a request scope" when
+    // invoked outside a request, and RLS would also hide the *recipient's*
+    // 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();
+
+    // Check user preferences (recipient row, read via the admin client above).
+    const prefs = await getUserNotificationPreferences(userId, supabase);
     if (!shouldSendPush(type, prefs)) {
       return { success: true }; // User opted out
     }
 
-    const supabase = await createClient();
-
     // Get user's active device tokens
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const { data: tokens, error: tokenError } = await (supabase as any)

src/test/lib/notifications/push.test.ts

@@ -0,0 +1,118 @@
+/**
+ * Regression test for src/lib/notifications/push.ts — `sendPushNotification`.
+ *
+ * Guards the production fix for:
+ *   "insight-notifier: push send reported failure: `cookies` was called
+ *    outside a request scope"
+ *
+ * sendPushNotification is invoked from BACKGROUND contexts (the post-write
+ * insight-notifier, the event-reminders cron) where there is no active request,
+ * so it must NOT use the cookie-backed request client. It now reads prefs +
+ * device tokens through the service-role admin client (the same key the
+ * send-apns-push Edge Function already uses), which also lets it read a
+ * *recipient's* device tokens when the sender differs from the recipient.
+ *
+ * The cookie client (`@/lib/supabase/server`) is mocked to THROW so the test
+ * fails loudly if any code path regresses back to it.
+ */
+
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { DEFAULT_NOTIFICATION_PREFERENCES } from '@/lib/notifications/types';
+
+// --- Controllable mock state ----------------------------------------------
+
+let prefs: Record<string, unknown> = { ...DEFAULT_NOTIFICATION_PREFERENCES, push_events: true };
+let deviceTokens: Array<{ token: string; platform: string }> = [];
+
+// The cookie-backed request client MUST NOT be touched in a background context.
+const serverCreateClient = vi.fn(async () => {
+  throw new Error('`cookies` was called outside a request scope');
+});
+vi.mock('@/lib/supabase/server', () => ({
+  createClient: serverCreateClient,
+}));
+
+// Service-role admin client — chainable fake for `users` + `device_tokens`.
+const adminFrom = vi.fn((table: string) => {
+  if (table === 'users') {
+    return {
+      select: () => ({
+        eq: () => ({
+          maybeSingle: async () => ({
+            data: { notification_preferences: prefs },
+            error: null,
+          }),
+        }),
+      }),
+    };
+  }
+  // device_tokens
+  return {
+    select: () => ({
+      eq: () => ({
+        // .from().select().eq('user_id').eq('active') is awaited directly.
+        eq: async () => ({ data: deviceTokens, error: null }),
+      }),
+    }),
+    update: () => ({
+      eq: async () => ({ data: null, error: null }),
+    }),
+  };
+});
+const createAdminClient = vi.fn(() => ({ from: adminFrom }));
+vi.mock('@/lib/supabase/admin', () => ({ createAdminClient }));
+
+// --- fetch mock -----------------------------------------------------------
+
+const fetchSpy = vi.fn(async () => ({ ok: true, text: async () => '' }));
+
+let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
+
+beforeEach(() => {
+  prefs = { ...DEFAULT_NOTIFICATION_PREFERENCES, push_events: true };
+  deviceTokens = [{ token: 'devicetoken123', platform: 'ios' }];
+  serverCreateClient.mockClear();
+  adminFrom.mockClear();
+  createAdminClient.mockClear();
+  fetchSpy.mockClear();
+  vi.stubGlobal('fetch', fetchSpy);
+  consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
+});
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+  consoleErrorSpy.mockRestore();
+});
+
+describe('sendPushNotification (background-safe)', () => {
+  it('delivers via the service-role admin client without touching the cookie request client', async () => {
+    const { sendPushNotification } = await import('@/lib/notifications/push');
+    const result = await sendPushNotification('coachhelm_insight', 'user-1', { insightTitle: 'x' });
+
+    expect(result.success).toBe(true);
+    expect(createAdminClient).toHaveBeenCalled();
+    // The regression guard: the no-request-scope cookie client is never used.
+    expect(serverCreateClient).not.toHaveBeenCalled();
+    expect(fetchSpy).toHaveBeenCalledTimes(1);
+  });
+
+  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();
+  });
+
+  it('returns success when the recipient has no active device tokens', async () => {
+    deviceTokens = [];
+    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();
+  });
+});

supabase/tests/rls/golf_coach_insights_cross_tenant_select.sql

@@ -83,9 +83,9 @@ BEGIN
     (id, coach_id, player_id, team_id, insight_type, title, content, priority, category)
   VALUES
     ('00000000-0000-0000-0000-0000000000af', v_coach_a, v_player_a, v_team_a,
-       'value_derived', 'A insight', 'tenant A only', 'high', 'putting'),
+       'putting', 'A insight', 'tenant A only', 'high', 'putting'),
     ('00000000-0000-0000-0000-0000000000bf', v_coach_b, v_player_b, v_team_b,
-       'value_derived', 'B insight', 'tenant B only', 'high', 'putting')
+       'putting', 'B insight', 'tenant B only', 'high', 'putting')
   ON CONFLICT DO NOTHING;
 END $$;