Prevent supabase role deletion and creation of roles by users without permission#41600
Prevent supabase role deletion and creation of roles by users without permission#41600marsou001 wants to merge 8 commits intosupabase:masterfrom
Conversation
|
Thanks for contributing to Supabase! ❤️ Our team will review your PR. A few tips for a smoother review process:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@marsou001 is attempting to deploy a commit to the Supabase Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdded a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserURL as "Browser (query state)"
participant RolesList
participant RoleRow
participant DeleteModal as "DeleteRoleModal"
participant API
participant Toast
Note over BrowserURL,RolesList: Deletion flow uses numeric `delete` query param
User->>BrowserURL: add ?delete=<id> or click delete
BrowserURL->>RolesList: query state `delete` updates (number)
RolesList->>RolesList: derive roleToDelete from roles lists
alt role found
RolesList->>DeleteModal: render with roleToDelete
User->>DeleteModal: confirm deletion
DeleteModal->>API: request delete(role.id)
API-->>DeleteModal: success / error
DeleteModal->>RolesList: close & reset delete query-state
DeleteModal->>Toast: show success / error
else role not found
RolesList->>Toast: show contextual error
RolesList->>BrowserURL: reset delete query-state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (1)
71-89: LGTM: Deletion logic correctly prevents Supabase role deletion.The
useQueryStateWithSelectimplementation properly prevents deletion of Supabase-managed roles via URL manipulation:
- The
selectfunction only searches inotherRoles(line 74), so Supabase roles are never selectable for deletion- The
onErrorcallback correctly identifies when a user attempts to delete a Supabase role (lines 77-79) and displays the appropriate error message- The
enabledcondition ensures the hook only operates when data is loadedThis successfully addresses the PR objective of preventing the
?delete=${roleId}bypass.Minor: Simplify redundant condition
The
id ?check on line 74 is redundant since theselectfunction is only invoked when a non-emptyselectedIdexists. Consider simplifying:- select: (id: string) => - id ? otherRoles?.find((role) => role.id.toString() === id) : undefined, + select: (id: string) => otherRoles?.find((role) => role.id.toString() === id),This is defensive programming and doesn't cause issues, so it's purely a style preference.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/studio/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.{ts,tsx}: Assign complex conditions to descriptive variables instead of using multiple conditions in a single expression
Use consistent naming conventions for boolean variables:isprefix for state/identity,hasprefix for possession,canprefix for capability/permission,shouldprefix for conditional behavior
Derive boolean state from existing state instead of storing it separately
Use early returns for guard clauses instead of deeply nested conditionals
Extract complex logic into custom hooks when logic becomes reusable or complex
Return objects from custom hooks instead of arrays for better extensibility and clearer API
Use discriminated unions for complex state management instead of multiple independent state fields
Avoid type casting (e.g.,as any,as Type); instead validate values at runtime using zod schemas
Files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
apps/studio/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.tsx: Components should ideally be under 200-300 lines; break down large components with multiple distinct UI sections, complex conditional rendering, or multiple unrelated useState hooks
Extract repeated JSX patterns into reusable components instead of copying similar JSX blocks
Use consistent loading/error/success pattern: handle loading state first with early returns, then error state, then empty state, then render success state
Keep state as local as possible and only lift up when needed
Group related state using objects or reducers instead of multiple useState calls, preferring react-hook-form for form state management
Name event handlers consistently: useonprefix for prop callbacks andhandleprefix for internal handlers
Avoid inline arrow functions for expensive operations; use useCallback to maintain stable function references
Use appropriate conditional rendering patterns: && for simple show/hide, ternary for binary choice, early returns or extracted components for multiple conditions
Avoid nested ternaries in JSX; use separate conditions or early returns instead
Use useMemo for expensive computations when the computation is genuinely expensive and the value is passed to memoized children
Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧠 Learnings (8)
📓 Common learnings
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:28:04.537Z
Learning: In supabase/supabase, use the existing 'do-not-merge' label to block merges for entitlement-gated UI changes until the backend entitlement is live and API types are regenerated.
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Handle dirty state by showing cancel buttons and disabling save buttons based on form.formState.isDirty
Applied to files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.tsx : Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Applied to files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/pages/**/*.{ts,tsx} : Add table actions to primary and secondary actions of PageLayout if table is main content of a page without search or filters
Applied to files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : When submit button is outside the form, add a formId variable outside the component, set it as id on form element and form prop on the button
Applied to files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Use FormItemLayout without form control for action fields with buttons for navigation or performable actions, wrapping buttons in a div with justify-end
Applied to files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Place submit/cancel buttons in SheetFooter for forms in sheets
Applied to files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx
📚 Learning: 2025-12-12T05:20:17.409Z
Learnt from: joshenlim
Repo: supabase/supabase PR: 41258
File: apps/studio/pages/project/[ref]/storage/vectors/buckets/[bucketId].tsx:9-9
Timestamp: 2025-12-12T05:20:17.409Z
Learning: In apps/studio/**/*.{ts,tsx}, use named imports for DefaultLayout: import { DefaultLayout } from 'components/layouts/DefaultLayout' instead of default import. This is the new practice being adopted across the studio app.
Applied to files:
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧬 Code graph analysis (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (3)
apps/studio/hooks/misc/useQueryStateWithSelect.ts (1)
useQueryStateWithSelect(17-46)apps/studio/components/interfaces/Settings/Logs/Logs.utils.ts (1)
role(741-757)apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx (1)
CreateRolePanel(48-191)
🔇 Additional comments (3)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (2)
66-69: LGTM: URL state management prevents creation bypass.The
isCreatingRolestate is now synced with the URL query parameternew, which properly handles the scenario where users manually add?new=trueto the URL. Combined with thedisabledprop passed toCreateRolePanel(line 242), this ensures that even if the panel opens via URL manipulation, users without permission cannot submit the form.
240-244: LGTM: Permission-based disabling prevents unauthorized creation.The
disabled={!canUpdateRoles}prop correctly prevents users without permission from submitting the role creation form, even if they bypass the disabled "Add role" button by manually adding?new=trueto the URL. This directly addresses the second bypass scenario described in the PR objectives.The combination of:
- URL state management (lines 66-69) that opens the panel
- Permission check that disables form actions (line 242)
...ensures that unauthorized users cannot create roles through any path.
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx (1)
24-24: Thedisabledprop correctly handles permission-based disabling.The prop is properly typed, destructured at line 48, and passed to
FormActionsat line 90. The component correctly prevents form submission when the user lacks permission—thedisabledprop overrides button state inFormActionsvia the conditionisSubmitting || disabled || (!hasChanges && hasChanges !== undefined), keeping buttons disabled regardless of form dirty state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsx (1)
27-27: Consider removing redundant null guards.The
roleprop is required per the interface, andRolesList.tsxonly renders this modal whenroleToDelete !== undefined. The guard on line 27 and optional chaining on line 43 are unnecessary.🔎 Proposed fix
const deleteRole = async () => { if (!project) return console.error('Project is required') - if (!role) return console.error('Failed to delete role: role is missing') onDelete?.() deleteDatabaseRole({- header={<h3>Confirm to delete role "{role?.name}"</h3>} + header={<h3>Confirm to delete role "{role.name}"</h3>}Also applies to: 43-43
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (1)
245-249: Redundantvisibleprop onDeleteRoleModal.Since the modal is conditionally rendered only when
roleToDelete !== undefined, thevisibleprop always evaluates totrue. Consider removing it or relying solely on conditional rendering.🔎 Proposed fix
- {roleToDelete !== undefined && <DeleteRoleModal - role={roleToDelete} - visible={roleToDelete !== undefined} - onClose={() => setRoleToDeleteId(null)} - />} + {roleToDelete !== undefined && ( + <DeleteRoleModal + role={roleToDelete} + visible + onClose={() => setRoleToDeleteId(null)} + /> + )}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsxapps/studio/components/interfaces/Database/Roles/RoleRow.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/studio/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.{ts,tsx}: Assign complex conditions to descriptive variables instead of using multiple conditions in a single expression
Use consistent naming conventions for boolean variables:isprefix for state/identity,hasprefix for possession,canprefix for capability/permission,shouldprefix for conditional behavior
Derive boolean state from existing state instead of storing it separately
Use early returns for guard clauses instead of deeply nested conditionals
Extract complex logic into custom hooks when logic becomes reusable or complex
Return objects from custom hooks instead of arrays for better extensibility and clearer API
Use discriminated unions for complex state management instead of multiple independent state fields
Avoid type casting (e.g.,as any,as Type); instead validate values at runtime using zod schemas
Files:
apps/studio/components/interfaces/Database/Roles/RoleRow.tsxapps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
apps/studio/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.tsx: Components should ideally be under 200-300 lines; break down large components with multiple distinct UI sections, complex conditional rendering, or multiple unrelated useState hooks
Extract repeated JSX patterns into reusable components instead of copying similar JSX blocks
Use consistent loading/error/success pattern: handle loading state first with early returns, then error state, then empty state, then render success state
Keep state as local as possible and only lift up when needed
Group related state using objects or reducers instead of multiple useState calls, preferring react-hook-form for form state management
Name event handlers consistently: useonprefix for prop callbacks andhandleprefix for internal handlers
Avoid inline arrow functions for expensive operations; use useCallback to maintain stable function references
Use appropriate conditional rendering patterns: && for simple show/hide, ternary for binary choice, early returns or extracted components for multiple conditions
Avoid nested ternaries in JSX; use separate conditions or early returns instead
Use useMemo for expensive computations when the computation is genuinely expensive and the value is passed to memoized children
Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Files:
apps/studio/components/interfaces/Database/Roles/RoleRow.tsxapps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:28:04.537Z
Learning: In supabase/supabase, use the existing 'do-not-merge' label to block merges for entitlement-gated UI changes until the backend entitlement is live and API types are regenerated.
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.tsx : Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Applied to files:
apps/studio/components/interfaces/Database/Roles/RoleRow.tsx
📚 Learning: 2025-12-12T05:20:17.409Z
Learnt from: joshenlim
Repo: supabase/supabase PR: 41258
File: apps/studio/pages/project/[ref]/storage/vectors/buckets/[bucketId].tsx:9-9
Timestamp: 2025-12-12T05:20:17.409Z
Learning: In apps/studio/**/*.{ts,tsx}, use named imports for DefaultLayout: import { DefaultLayout } from 'components/layouts/DefaultLayout' instead of default import. This is the new practice being adopted across the studio app.
Applied to files:
apps/studio/components/interfaces/Database/Roles/RoleRow.tsxapps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsxapps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/pages/**/*.{ts,tsx} : Add table actions to primary and secondary actions of PageLayout if table is main content of a page without search or filters
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.{ts,tsx} : Use discriminated unions for complex state management instead of multiple independent state fields
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧬 Code graph analysis (3)
apps/studio/components/interfaces/Database/Roles/RoleRow.tsx (1)
apps/studio/components/interfaces/Settings/Logs/Logs.utils.ts (1)
role(741-757)
apps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsx (1)
apps/studio/data/database-roles/database-roles-query.ts (1)
PgRole(14-14)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (2)
apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx (1)
CreateRolePanel(48-191)apps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsx (1)
DeleteRoleModal(15-54)
🔇 Additional comments (5)
apps/studio/components/interfaces/Database/Roles/RoleRow.tsx (2)
24-28: LGTM! Type alignment for numeric role ID.The prop type update from
stringtonumbercorrectly aligns withPgRole.id's type and the updated deletion state management inRolesList.tsx.
140-145: LGTM! Simplified callback invocation.Passing
role.iddirectly removes unnecessary type conversion and is consistent with the updated prop signature.apps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsx (1)
1-1: LGTM! Type migration toPgRole.The change from
PostgresRoletoPgRolealigns with the Zod-inferred type fromdatabase-roles-query.tsand improves consistency across the roles feature.Also applies to: 8-9
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (2)
64-72: LGTM! URL state management for role creation and deletion.Using
nuqswithparseAsBooleanandparseAsIntegerfor URL state provides type-safe query parameter handling with appropriate defaults and history management.
239-243: LGTM! Properly gates role creation panel with permissions.Passing
disabled={!canUpdateRoles}toCreateRolePanelensures that users who bypass the button via URL manipulation (?new=true) cannot interact with the form actions, addressing the linked issue #41599.
| useEffect(() => { | ||
| if (!isLoading && roleToDeleteId !== null && roleToDelete === undefined) { | ||
| const isSupabaseRole = | ||
| supabaseRoles.find((role) => role.id === roleToDeleteId) !== undefined | ||
| if (isSupabaseRole) { | ||
| toast.error('Cannot delete role as it is a Supabase role') | ||
| } else { | ||
| toast.error('Database Role not found') | ||
| } | ||
|
|
||
| setRoleToDeleteId(null) | ||
| } | ||
| }, [isLoading, roleToDeleteId, roleToDelete]) |
There was a problem hiding this comment.
Add missing dependencies to useEffect.
The dependency array is missing supabaseRoles and setRoleToDeleteId. While setRoleToDeleteId from useQueryState is likely stable, supabaseRoles is derived fresh on each render and could cause stale closure issues if the data changes while the effect is pending.
🔎 Proposed fix
useEffect(() => {
if (!isLoading && roleToDeleteId !== null && roleToDelete === undefined) {
const isSupabaseRole =
supabaseRoles.find((role) => role.id === roleToDeleteId) !== undefined
if (isSupabaseRole) {
toast.error('Cannot delete role as it is a Supabase role')
} else {
toast.error('Database Role not found')
}
setRoleToDeleteId(null)
}
- }, [isLoading, roleToDeleteId, roleToDelete])
+ }, [isLoading, roleToDeleteId, roleToDelete, supabaseRoles, setRoleToDeleteId])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!isLoading && roleToDeleteId !== null && roleToDelete === undefined) { | |
| const isSupabaseRole = | |
| supabaseRoles.find((role) => role.id === roleToDeleteId) !== undefined | |
| if (isSupabaseRole) { | |
| toast.error('Cannot delete role as it is a Supabase role') | |
| } else { | |
| toast.error('Database Role not found') | |
| } | |
| setRoleToDeleteId(null) | |
| } | |
| }, [isLoading, roleToDeleteId, roleToDelete]) | |
| useEffect(() => { | |
| if (!isLoading && roleToDeleteId !== null && roleToDelete === undefined) { | |
| const isSupabaseRole = | |
| supabaseRoles.find((role) => role.id === roleToDeleteId) !== undefined | |
| if (isSupabaseRole) { | |
| toast.error('Cannot delete role as it is a Supabase role') | |
| } else { | |
| toast.error('Database Role not found') | |
| } | |
| setRoleToDeleteId(null) | |
| } | |
| }, [isLoading, roleToDeleteId, roleToDelete, supabaseRoles, setRoleToDeleteId]) |
🤖 Prompt for AI Agents
In apps/studio/components/interfaces/Database/Roles/RolesList.tsx around lines
76 to 88, the useEffect callback references supabaseRoles and setRoleToDeleteId
but they are not listed in the dependency array causing potential stale
closures; update the dependency array to include supabaseRoles and
setRoleToDeleteId (even if stable) so the effect re-runs when role lists or the
setter change, ensuring the toast logic and reset of roleToDeleteId use current
values.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (1)
30-30: Fix ref logic bug and dependency array issues.The
deletingRoleIdRefpattern has two critical flaws:
Ref is never reset: After a deletion completes (Lines 251–255 set
deletingRoleIdRef.current = roleToDeleteId), the ref retains that ID indefinitely. If a user later manually navigates to?delete=<same-id>after the role is deleted, the condition on Line 84 (deletingRoleIdRef.current === null) will be false, preventing the error toast from displaying.Anti-pattern in dependency array (Line 95): Including
deletingRoleIdRef.currenthas no effect—refs don't cause re-renders. Remove it from the array.Missing dependencies (flagged in past review):
supabaseRolesandsetRoleToDeleteIdshould be included.🔎 Recommended fix
Change the ref check to compare with the current
roleToDeleteId, and fix the dependency array:useEffect(() => { - if ( - !isLoading && - roleToDeleteId !== null && - roleToDelete === undefined && - deletingRoleIdRef.current === null - ) { + if (!isLoading && roleToDeleteId !== null && roleToDelete === undefined) { + // Only show error if this deletion attempt is not the one currently in progress + if (deletingRoleIdRef.current !== roleToDeleteId) { const isSupabaseRole = supabaseRoles.find((role) => role.id === roleToDeleteId) !== undefined if (isSupabaseRole) { toast.error('Cannot delete role as it is a Supabase role') } else { toast.error('Database Role not found') } setRoleToDeleteId(null) + } } - }, [isLoading, roleToDeleteId, roleToDelete, deletingRoleIdRef.current]) + }, [isLoading, roleToDeleteId, roleToDelete, supabaseRoles, setRoleToDeleteId])This ensures the error displays correctly for all invalid deletion attempts, including repeated attempts on the same deleted role.
Also applies to: 79-95, 251-255
🧹 Nitpick comments (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (1)
247-250: Avoid type assertion; the visibility check already ensures safety.Line 248 uses
as PGRole, which violates the coding guideline to avoid type casting. Sincevisible={roleToDelete !== undefined}guarantees thatroleToDeleteis defined when the modal renders, the cast is redundant and could mask type issues.🔎 Refactor options
Option 1: Make
DeleteRoleModal.roleoptional and handleundefinedinternally:<DeleteRoleModal - role={roleToDelete as PGRole} + role={roleToDelete} visible={roleToDelete !== undefined}Option 2: Use non-null assertion with a comment explaining the visibility guard:
<DeleteRoleModal - role={roleToDelete as PGRole} + role={roleToDelete!} // Safe: visibility ensures roleToDelete is defined visible={roleToDelete !== undefined}Option 1 is preferred as it avoids all assertions.
As per coding guidelines: avoid type casting and validate values at runtime.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/studio/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.{ts,tsx}: Assign complex conditions to descriptive variables instead of using multiple conditions in a single expression
Use consistent naming conventions for boolean variables:isprefix for state/identity,hasprefix for possession,canprefix for capability/permission,shouldprefix for conditional behavior
Derive boolean state from existing state instead of storing it separately
Use early returns for guard clauses instead of deeply nested conditionals
Extract complex logic into custom hooks when logic becomes reusable or complex
Return objects from custom hooks instead of arrays for better extensibility and clearer API
Use discriminated unions for complex state management instead of multiple independent state fields
Avoid type casting (e.g.,as any,as Type); instead validate values at runtime using zod schemas
Files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
apps/studio/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.tsx: Components should ideally be under 200-300 lines; break down large components with multiple distinct UI sections, complex conditional rendering, or multiple unrelated useState hooks
Extract repeated JSX patterns into reusable components instead of copying similar JSX blocks
Use consistent loading/error/success pattern: handle loading state first with early returns, then error state, then empty state, then render success state
Keep state as local as possible and only lift up when needed
Group related state using objects or reducers instead of multiple useState calls, preferring react-hook-form for form state management
Name event handlers consistently: useonprefix for prop callbacks andhandleprefix for internal handlers
Avoid inline arrow functions for expensive operations; use useCallback to maintain stable function references
Use appropriate conditional rendering patterns: && for simple show/hide, ternary for binary choice, early returns or extracted components for multiple conditions
Avoid nested ternaries in JSX; use separate conditions or early returns instead
Use useMemo for expensive computations when the computation is genuinely expensive and the value is passed to memoized children
Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧠 Learnings (9)
📓 Common learnings
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:28:04.537Z
Learning: In supabase/supabase, use the existing 'do-not-merge' label to block merges for entitlement-gated UI changes until the backend entitlement is live and API types are regenerated.
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.tsx : Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/pages/**/*.{ts,tsx} : Add table actions to primary and secondary actions of PageLayout if table is main content of a page without search or filters
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : When doing a mutation, always use the mutate function with onSuccess and onError callbacks that include toast.success and toast.error
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Contain tables within a card
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.{ts,tsx} : Use discriminated unions for complex state management instead of multiple independent state fields
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.tsx : Avoid inline arrow functions for expensive operations; use useCallback to maintain stable function references
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Use mutateAsync only if the mutation is part of multiple async actions, wrapping the call with try/catch block and adding toast.success and toast.error
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-12T05:20:17.409Z
Learnt from: joshenlim
Repo: supabase/supabase PR: 41258
File: apps/studio/pages/project/[ref]/storage/vectors/buckets/[bucketId].tsx:9-9
Timestamp: 2025-12-12T05:20:17.409Z
Learning: In apps/studio/**/*.{ts,tsx}, use named imports for DefaultLayout: import { DefaultLayout } from 'components/layouts/DefaultLayout' instead of default import. This is the new practice being adopted across the studio app.
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧬 Code graph analysis (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (4)
apps/studio/components/interfaces/Settings/Logs/Logs.utils.ts (1)
role(741-757)apps/studio/components/interfaces/Database/Roles/RoleRow.tsx (1)
RoleRow(30-221)apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx (1)
CreateRolePanel(48-191)apps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsx (1)
DeleteRoleModal(15-54)
🔇 Additional comments (5)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (5)
1-1: LGTM: Imports support the new URL-based state management.The new imports—
PGRolefor type safety,useQueryStatewith parsers for URL state,useEffect/useReffor side effects, andtoastfor user feedback—are appropriate for the deletion/creation flow changes.Also applies to: 5-7
66-74: LGTM: URL query state configuration is appropriate.Using
parseAsBooleanforisCreatingRoleandparseAsIntegerforroleToDeleteIdwithhistory: 'push'andclearOnDefault: truecorrectly establishes URL-backed state for role creation and deletion flows.
76-76: LGTM: Derived state correctly restricts deletion to non-Supabase roles.Deriving
roleToDeletefromotherRoles.find(...)ensures only non-Supabase-managed roles can be deleted, aligning with the PR objective.
215-215: LGTM: RoleRow deletion handlers correctly wired.Passing
setRoleToDeleteIdtoonSelectDeletealigns with the new ID-based deletion flow. According to the relevant code snippets,RoleRowinvokesonSelectDelete(role.id)(a number), which matches the expected signature.Also applies to: 231-231
241-245: LGTM: Correctly gates role creation based on permissions.Passing
disabled={!canUpdateRoles}toCreateRolePanelensures users lacking creation permissions cannot interact with the role-creation form, even if they bypass the disabled "Add role" button by manually adding?new=trueto the URL. This directly addresses the second PR objective.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (1)
78-95: Add missing dependencies to useEffect.The dependency array is missing
supabaseRolesandsetRoleToDeleteId. WhilesetRoleToDeleteIdfromuseQueryStateis likely stable,supabaseRolesis derived fresh on each render and could cause stale closure issues.
🧹 Nitpick comments (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (1)
247-256: Avoid type assertion; use conditional rendering instead.Per coding guidelines, type casting should be avoided. While the cast is safe at runtime (modal is only visible when
roleToDeleteis defined), you can eliminate the assertion by conditionally rendering the modal.🔎 Proposed fix
- <DeleteRoleModal - role={roleToDelete as PGRole} - visible={roleToDelete !== undefined} - onClose={() => setRoleToDeleteId(null)} - onDelete={() => { - if (roleToDelete) { - deletingRoleIdRef.current = roleToDeleteId - } - }} - /> + {roleToDelete !== undefined && ( + <DeleteRoleModal + role={roleToDelete} + visible + onClose={() => setRoleToDeleteId(null)} + onDelete={() => { + deletingRoleIdRef.current = roleToDeleteId + }} + /> + )}This removes the type assertion and the redundant
if (roleToDelete)check insideonDelete.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/studio/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.{ts,tsx}: Assign complex conditions to descriptive variables instead of using multiple conditions in a single expression
Use consistent naming conventions for boolean variables:isprefix for state/identity,hasprefix for possession,canprefix for capability/permission,shouldprefix for conditional behavior
Derive boolean state from existing state instead of storing it separately
Use early returns for guard clauses instead of deeply nested conditionals
Extract complex logic into custom hooks when logic becomes reusable or complex
Return objects from custom hooks instead of arrays for better extensibility and clearer API
Use discriminated unions for complex state management instead of multiple independent state fields
Avoid type casting (e.g.,as any,as Type); instead validate values at runtime using zod schemas
Files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
apps/studio/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/studio-best-practices.mdc)
apps/studio/**/*.tsx: Components should ideally be under 200-300 lines; break down large components with multiple distinct UI sections, complex conditional rendering, or multiple unrelated useState hooks
Extract repeated JSX patterns into reusable components instead of copying similar JSX blocks
Use consistent loading/error/success pattern: handle loading state first with early returns, then error state, then empty state, then render success state
Keep state as local as possible and only lift up when needed
Group related state using objects or reducers instead of multiple useState calls, preferring react-hook-form for form state management
Name event handlers consistently: useonprefix for prop callbacks andhandleprefix for internal handlers
Avoid inline arrow functions for expensive operations; use useCallback to maintain stable function references
Use appropriate conditional rendering patterns: && for simple show/hide, ternary for binary choice, early returns or extracted components for multiple conditions
Avoid nested ternaries in JSX; use separate conditions or early returns instead
Use useMemo for expensive computations when the computation is genuinely expensive and the value is passed to memoized children
Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧠 Learnings (7)
📓 Common learnings
Learnt from: ignaciodob
Repo: supabase/supabase PR: 41291
File: packages/api-types/types/platform.d.ts:7128-7128
Timestamp: 2025-12-12T11:28:04.537Z
Learning: In supabase/supabase, use the existing 'do-not-merge' label to block merges for entitlement-gated UI changes until the backend entitlement is live and API types are regenerated.
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : When doing a mutation, always use the mutate function with onSuccess and onError callbacks that include toast.success and toast.error
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/pages/**/*.{ts,tsx} : Add table actions to primary and secondary actions of PageLayout if table is main content of a page without search or filters
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.tsx : Define prop interfaces explicitly for React components with proper typing of props and callback handlers
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T17:04:40.037Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-ui.mdc:0-0
Timestamp: 2025-12-11T17:04:40.037Z
Learning: Applies to apps/studio/components/**/*.{ts,tsx} : Use mutateAsync only if the mutation is part of multiple async actions, wrapping the call with try/catch block and adding toast.success and toast.error
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-11T16:46:18.701Z
Learnt from: CR
Repo: supabase/supabase PR: 0
File: .cursor/rules/studio-best-practices.mdc:0-0
Timestamp: 2025-12-11T16:46:18.701Z
Learning: Applies to apps/studio/**/*.tsx : Avoid inline arrow functions for expensive operations; use useCallback to maintain stable function references
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
📚 Learning: 2025-12-12T05:20:17.409Z
Learnt from: joshenlim
Repo: supabase/supabase PR: 41258
File: apps/studio/pages/project/[ref]/storage/vectors/buckets/[bucketId].tsx:9-9
Timestamp: 2025-12-12T05:20:17.409Z
Learning: In apps/studio/**/*.{ts,tsx}, use named imports for DefaultLayout: import { DefaultLayout } from 'components/layouts/DefaultLayout' instead of default import. This is the new practice being adopted across the studio app.
Applied to files:
apps/studio/components/interfaces/Database/Roles/RolesList.tsx
🧬 Code graph analysis (1)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (3)
apps/studio/components/interfaces/Database/Roles/RoleRow.tsx (1)
RoleRow(30-221)apps/studio/components/interfaces/Database/Roles/CreateRolePanel.tsx (1)
CreateRolePanel(48-191)apps/studio/components/interfaces/Database/Roles/DeleteRoleModal.tsx (1)
DeleteRoleModal(15-54)
🔇 Additional comments (4)
apps/studio/components/interfaces/Database/Roles/RolesList.tsx (4)
1-7: LGTM!The imports are appropriately added to support the new URL state management with
nuqs, validation logic withuseEffect, and user feedback withtoast.
214-216: LGTM!Supabase-managed roles are correctly rendered with
disabledto prevent modification, and the delete option won't be shown in the dropdown.
226-233: LGTM!Permission-based disabling is correctly applied, and the delete handler properly sets the URL state.
241-245: LGTM!The
disabledprop correctly prevents users without permission from submitting the form even if they bypass the button by adding?new=trueto the URL. This addresses the linked issue.
2 participants
I have read the CONTRIBUTING.md file.
YES
What kind of change does this PR introduce?
Bug fix, resolves #41599
What is the new behavior?
if the user manually adds
delete=${roleId}query param to the url, androleIdis the role id of a Supabase role, the confirmation dialog won't appear. Instead, the user will get a toast message saying "Cannot delete role as it is a Supabase role".if the user manually adds
new=truequery param to the url, and he/she isn't allowed to create roles, the action buttons of the side panel ("cancel", "save") are disabled, whether the form values change or not.Edit: removed
useQueryStateWithSelect.Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.