Fix/supabase service role#145
Conversation
…ice role key; refactor DashboardLayout to use server Supabase client directly.
basanth-p
left a comment
There was a problem hiding this comment.
Review — Fix/supabase-service-role
The fix is directionally correct — removing the getAdminClient() + service-role dependency from DashboardLayout is the right call, and the contributor DX improvement (.env.example reorg + PostHog vars) is welcome. However, there's one correctness issue and one security concern that need to be addressed before merging.
🔴 Critical — Must Fix
1. profiles table query switched from service-role to user-scoped client — RLS must allow it
The core change replaces admin.from("profiles") with supabase.from("profiles"), where supabase is the user-scoped createServerSupabaseClient() client. This is cleaner, but it only works if your Row Level Security (RLS) policy on the profiles table allows the authenticated user to SELECT their own row.
If profiles has no RLS policy (or has a restrictive policy), this query will silently return { data: null, error: null } — causing every authenticated user to be treated as non-onboarded and redirected to /onboard in a loop.
Please confirm one of the following before merging:
- ✅ There is an RLS policy on
profileslikeUSING (auth.uid() = id)forSELECT - ✅ RLS is disabled on
profiles(acceptable for a dev project, should be noted) - Or add a migration/comment documenting the required RLS policy
2. error from the profile query is no longer handled
The old code used getAdminClient() which at least threw on misconfiguration. The new query reads:
const { data: profile, error } = await supabase
.from("profiles")
.select("is_onboarded")
.eq("id", user.id)But error is destructured and then never checked. If the query fails (RLS denial, network error, table doesn't exist), profile will be null and the user silently falls through without any signal. At minimum add:
if (error) {
console.error("[DashboardLayout] Failed to fetch profile:", error.message);
}🟡 Notable — Should Address
3. getAdminClient() removed — confirm no other usage
The function is deleted entirely. Make sure no other server component or API route was calling getAdminClient() from this file. If it was used elsewhere, it should be extracted to a shared utility (e.g. lib/supabase/admin.ts) rather than deleted. A quick grep for getAdminClient across the codebase would confirm this is safe.
🟢 Good Changes
.env.examplereorg — MovingSUPABASE_SERVICE_ROLE_KEYunder a# Server-onlycomment with clear context is a great DX improvement. Contributors will no longer be confused about which vars are required for local dev. ✅- PostHog vars added — Consistent with PR #139's additions; empty defaults with region comments are the right approach. ✅
- Removing the service-role client from
DashboardLayout— Using the service-role key in a layout component that runs on every dashboard render was unnecessarily privileged. The user-scoped client is the correct pattern here, assuming RLS is in place. ✅
Summary: Confirm RLS on profiles allows SELECT by the row owner (#1), and add a basic error check on the profile query (#2). Both are quick fixes. Everything else looks good.
basanth-p
left a comment
There was a problem hiding this comment.
Review — Fix/supabase-service-role
The direction is right — removing the service-role client from DashboardLayout is the correct architectural move, and the .env.example cleanup is a great contributor DX improvement. Two blockers need to be addressed before this can merge.
🔴 Blockers (2)
- RLS on
profiles— the switch to a user-scoped client only works if aSELECTRLS policy exists for the row owner. No confirmation or migration is included. See inline comment on line 15. errornever checked — a failed query silently treats all users as non-onboarded. See inline comment on line 12.
🟡 Notable (1)
- Confirm
getAdminClient()has no other callers before deleting it. See inline comment.
✅ Good
.env.exampleSUPABASE_SERVICE_ROLE_KEYmoved under# Server-onlywith clear context- PostHog vars added with empty defaults + region comments
- Removing service-role from a layout component that runs on every dashboard render is the right call
| const admin = getAdminClient(); | ||
| const { data: profile, error } = await admin | ||
| const { data: profile, error } = await supabase | ||
| .from("profiles") |
There was a problem hiding this comment.
🔴 Blocker: RLS on profiles must allow SELECT for this to work
The switch from admin.from("profiles") (service-role, bypasses RLS) to supabase.from("profiles") (user-scoped, respects RLS) is architecturally correct — but it only works if there is an RLS policy on the profiles table that allows each user to select their own row.
Without an RLS policy like:
CREATE POLICY "Users can read own profile"
ON profiles FOR SELECT
USING (auth.uid() = id);...this query will silently return { data: null, error: null }, causing every authenticated user to be treated as non-onboarded and get redirect-looped to /onboard.
Please confirm one of:
- ✅ This RLS
SELECTpolicy already exists onprofiles - ✅ RLS is intentionally disabled on
profiles(note it in a comment) - Or add the migration / policy to the PR
| @@ -27,8 +11,7 @@ export default async function DashboardLayout({ children }: { children: React.Re | |||
| redirect("/auth/login?redirect=/dashboard"); | |||
| } | |||
There was a problem hiding this comment.
🔴 Blocker: error is destructured but never checked
error is captured in the destructure but the code never acts on it. If this query fails (RLS denial, network error, table name mismatch), profile will be null and the user silently falls through — treated as non-onboarded with no log or signal anywhere.
Add a basic error check:
const { data: profile, error: profileError } = await supabase
.from("profiles")
.select("is_onboarded")
.eq("id", user.id)
.single();
if (profileError) {
console.error("[DashboardLayout] Failed to fetch profile:", profileError.message);
}Also note: .single() is appropriate here since you're querying by primary key — it returns the row directly (or an error) instead of an array, which avoids silent empty-array cases.
| @@ -1,21 +1,5 @@ | |||
| import { redirect } from "next/navigation"; | |||
| import { createServerSupabaseClient } from "@/lib/supabase/server"; | |||
There was a problem hiding this comment.
🟡 Notable: confirm getAdminClient() isn’t used anywhere else before deleting
This entire function is removed in this PR. Before merging, do a quick grep across the codebase:
grep -r "getAdminClient" .If any other server component or API route was importing/calling it from this file, those will silently break. If there are other callers, extract the function to a shared utility (e.g. lib/supabase/admin.ts) rather than deleting it outright.
| NEXT_PUBLIC_SUPABASE_ANON_KEY=your-local-supabase-anon-key | ||
| NEXT_PUBLIC_SITE_URL=http://localhost:3000 | ||
|
|
||
| # Server-only — required for /api/profile (Supabase Dashboard → API → service_role) |
There was a problem hiding this comment.
✅ Good change — Moving SUPABASE_SERVICE_ROLE_KEY under a # Server-only section with clear context ("required for /api/profile") is a meaningful DX improvement. Contributors will immediately understand this is not needed for basic local dev, reducing 500 errors like the one this PR fixes.
…e client usage in profile API; enhance error handling in DashboardLayout for profile fetching; add profiles table with RLS policies in schema.sql.
Fixes a 500 on /dashboard after login when SUPABASE_SERVICE_ROLE_KEY is not set in local env.
After the dev merge, DashboardLayout used a service-role admin client (getAdminClient()) to read profiles.is_onboarded. That threw Missing Supabase server configuration for contributors who only had the public Supabase vars in .env.local — a common local setup.