feat: migrate SDK and admin to use atomic member role RPCs#1508
feat: migrate SDK and admin to use atomic member role RPCs#1508whoAbhishekSah merged 16 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces client-side policy CRUD (listPolicies/deletePolicy/createPolicy/createPolicyForProject) with direct role-based mutations ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Manual Testing — PASSTested locally with client demo app on clean Frontier + SpiceDB.
All flows use |
whoAbhishekSah
requested review from
rohanchkrabrty and
rsbh
and removed request for
rohanchkrabrty
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/sdk/admin/views/organizations/details/projects/members/assign-role.tsx (1)
88-107:⚠️ Potential issue | 🔴 Critical
setProjectMemberRoleonly preserves the last role in this loop.This RPC replaces the member’s project role set; it does not append to it. Iterating
assignedRolesArrhere means each call overwrites the previous one, so only the last role survives whileonRoleUpdatestill reports every selected role locally.Suggested fix
const onSubmit = async (data: FormData) => { try { const assignedRolesArr = Array.from(data.roleIds); + if (assignedRolesArr.length !== 1) { + throw new Error("Project members can only have one direct project role"); + } - for (const roleId of assignedRolesArr) { - await setProjectMemberRole( - create(SetProjectMemberRoleRequestSchema, { - projectId, - principalId: user?.id || "", - principalType: "app/user", - roleId, - }), - ); - } + await setProjectMemberRole( + create(SetProjectMemberRoleRequestSchema, { + projectId, + principalId: user?.id || "", + principalType: "app/user", + roleId: assignedRolesArr[0], + }), + );web/sdk/react/views/projects/details/remove-project-member-dialog.tsx (1)
20-25:⚠️ Potential issue | 🟠 MajorMake
memberTyperequired instead of defaulting to'user'.This makes missing caller updates look valid.
web/sdk/react/views/projects/details/project-detail-page.tsx:255-260still opens the dialog withoutmemberType, so removing a team member will continue to sendapp/userand fail. Making the prop required forces the remaining call sites to pass the correct principal type.Suggested fix
export interface RemoveProjectMemberDialogProps { open: boolean; onOpenChange?: (value: boolean) => void; projectId: string; memberId: string; - memberType?: 'user' | 'group'; + memberType: 'user' | 'group'; } @@ export const RemoveProjectMemberDialog = ({ open, onOpenChange, projectId, memberId, - memberType = 'user' + memberType }: RemoveProjectMemberDialogProps) => {Also applies to: 32-33, 52-57
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96b266ff-0062-46b1-bc94-f27ec9d10147
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
web/sdk/admin/views/organizations/details/projects/members/assign-role.tsxweb/sdk/admin/views/organizations/details/projects/members/remove-member.tsxweb/sdk/admin/views/organizations/details/projects/use-add-project-members.tsxweb/sdk/package.jsonweb/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsxweb/sdk/react/views/api-keys/list/add-service-account-dialog.tsxweb/sdk/react/views/projects/details/project-member-columns.tsxweb/sdk/react/views/projects/details/project-members.tsxweb/sdk/react/views/projects/details/remove-project-member-dialog.tsx
web/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsx
Show resolved
Hide resolved
whoAbhishekSah
force-pushed
the
feat/sdk-project-member-management
branch
from
6e6778f to
d873d1e
Compare
Coverage Report for CI Build 24237209757Coverage remained the same at 41.337%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
web/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsx (1)
179-223:⚠️ Potential issue | 🟠 MajorAdd
ownerRoleIdto the callback dependencies.
onAccessChangereadsownerRoleIdon Lines 188 and 194, but Line 223 omits it. WhenlistRolesresolves after the dialog opens, the callback can keep the initial empty string and continue throwing “Project owner role not found” on grant attempts until another dependency changes.Suggested fix
- [serviceUserId, setProjectMemberRole, removeProjectMember] + [serviceUserId, ownerRoleId, setProjectMemberRole, removeProjectMember]
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cfd1e26-ef92-4f05-9d62-76ad0532fd3c
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
web/sdk/admin/views/organizations/details/projects/members/assign-role.tsxweb/sdk/admin/views/organizations/details/projects/members/remove-member.tsxweb/sdk/admin/views/organizations/details/projects/use-add-project-members.tsxweb/sdk/package.jsonweb/sdk/react/views-new/projects/components/add-member-menu.tsxweb/sdk/react/views-new/projects/components/remove-member-dialog.tsxweb/sdk/react/views-new/projects/project-details-view.tsxweb/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsxweb/sdk/react/views/api-keys/list/add-service-account-dialog.tsxweb/sdk/react/views/projects/details/project-member-columns.tsxweb/sdk/react/views/projects/details/project-members.tsxweb/sdk/react/views/projects/details/remove-project-member-dialog.tsx
✅ Files skipped from review due to trivial changes (3)
- web/sdk/package.json
- web/sdk/react/views/projects/details/project-member-columns.tsx
- web/sdk/react/views/api-keys/list/add-service-account-dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/sdk/admin/views/organizations/details/projects/members/remove-member.tsx
web/sdk/admin/views/organizations/details/projects/members/assign-role.tsx
Outdated
Show resolved
Hide resolved
web/sdk/react/views/projects/details/remove-project-member-dialog.tsx
Outdated
Show resolved
Hide resolved
whoAbhishekSah
force-pushed
the
feat/sdk-project-member-management
branch
from
c7c1ca0 to
cfeb60b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/sdk/react/views/api-keys/list/add-service-account-dialog.tsx (2)
128-153:⚠️ Potential issue | 🟠 MajorValidate
ownerRoleIdbefore creating the service user.The validation at line 145 occurs after
createServiceUser(line 133). IfownerRoleIdis empty (e.g., roles query is slow or the role name doesn't match), the service user will be created but left orphaned without project access.Suggested fix: validate early
const onSubmit = useCallback( async (data: FormData) => { if (!orgId) return; + if (!ownerRoleId) { + toast.error('Something went wrong', { + description: 'Project owner role not found' + }); + return; + } try { const serviceUserResponse = await createServiceUser( create(CreateServiceUserRequestSchema, { orgId, body: create(ServiceUserRequestBodySchema, { title: data.title }) }) ); const serviceUserId = serviceUserResponse.serviceuser?.id; if (!serviceUserId) return; - if (!ownerRoleId) throw new Error('Project owner role not found'); await setProjectMemberRole(
201-209:⚠️ Potential issue | 🟠 MajorAdd
ownerRoleIdto the dependency array.The callback uses
ownerRoleId(lines 145, 151) but it's missing from the dependency array. This causes a stale closure: the callback captures the initial empty string even afterlistRolesresolves, leading to "Project owner role not found" errors.Suggested fix
[ orgId, + ownerRoleId, createServiceUser, setProjectMemberRole, createServiceUserToken, queryClient, transport, onCreated ]web/sdk/admin/views/organizations/details/projects/use-add-project-members.tsx (1)
68-89:⚠️ Potential issue | 🟡 MinorSilent failure when
viewerRoleIdis missing.The early return at line 70 silently exits without user feedback if
viewerRoleIdis empty (e.g., role not found or query still loading). Users clicking "Add" see no response.Consider adding a toast or ensuring the add button is disabled while roles are loading.
Suggested fix
const addMember = useCallback( async (userId: string) => { - if (!userId || !projectId || !viewerRoleId) return; + if (!userId || !projectId) return; + if (!viewerRoleId) { + toast.error("Unable to add member", { description: "Project viewer role not found" }); + return; + } try {
♻️ Duplicate comments (1)
web/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsx (1)
179-224:⚠️ Potential issue | 🟠 Major
onAccessChangecloses over a staleownerRoleId.The callback uses
ownerRoleIdat lines 188 and 194, but it's missing from the dependency array at line 223. When the dialog opens andlistRolespopulates the role ID,onAccessChangestill holds the stale empty string from initial render, causing "Project owner role not found" errors.Suggested fix
- [serviceUserId, setProjectMemberRole, removeProjectMember] + [serviceUserId, ownerRoleId, setProjectMemberRole, removeProjectMember]
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3466a0ec-412a-4b4e-a218-712d1f8d0ba5
📒 Files selected for processing (11)
web/sdk/admin/views/organizations/details/projects/members/assign-role.tsxweb/sdk/admin/views/organizations/details/projects/members/remove-member.tsxweb/sdk/admin/views/organizations/details/projects/use-add-project-members.tsxweb/sdk/react/views-new/projects/components/add-member-menu.tsxweb/sdk/react/views-new/projects/components/remove-member-dialog.tsxweb/sdk/react/views-new/projects/project-details-view.tsxweb/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsxweb/sdk/react/views/api-keys/list/add-service-account-dialog.tsxweb/sdk/react/views/projects/details/project-member-columns.tsxweb/sdk/react/views/projects/details/project-members.tsxweb/sdk/react/views/projects/details/remove-project-member-dialog.tsx
✅ Files skipped from review due to trivial changes (2)
- web/sdk/react/views-new/projects/components/remove-member-dialog.tsx
- web/sdk/react/views/projects/details/project-member-columns.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/sdk/react/views-new/projects/components/add-member-menu.tsx
- web/sdk/react/views/projects/details/project-members.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/membership-package-tasks.md (1)
31-41: Consider adding language specifiers to fenced code blocks.Several code blocks use plain triple-backticks without a language identifier. While these are pseudo-code/call-chain diagrams rather than compilable code, adding a language hint (e.g.,
```textor```plaintext) would satisfy markdown linters and improve rendering consistency.Example fix for one block
-``` -Before: +```text +Before:Also applies to: 180-218, 222-234, 323-344, 400-432, 497-515, 586-600
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89a2c7f1-7a7c-4a9b-9b88-c6324742f82d
📒 Files selected for processing (4)
docs/membership-package-tasks.mdweb/sdk/react/views-new/projects/components/remove-member-dialog.tsxweb/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsxweb/sdk/react/views/api-keys/list/add-service-account-dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/sdk/react/views-new/projects/components/remove-member-dialog.tsx
- web/sdk/react/views/api-keys/list/add-service-account-dialog.tsx
whoAbhishekSah
force-pushed
the
feat/sdk-project-member-management
branch
from
8a6eecd to
d8baeb5
Compare
whoAbhishekSah
force-pushed
the
feat/sdk-project-member-management
branch
from
3cacd59 to
89bb65e
Compare
whoAbhishekSah
force-pushed
the
feat/sdk-project-member-management
branch
from
16bbb14 to
c7a2612
Compare
web/sdk/admin/views/organizations/details/projects/members/assign-role.tsx
Outdated
Show resolved
Hide resolved
web/sdk/react/views-new/projects/components/remove-member-dialog.tsx
Outdated
Show resolved
Hide resolved
web/sdk/react/views/api-keys/details/manage-service-user-projects-dialog.tsx
Outdated
Show resolved
Hide resolved
web/sdk/react/views/api-keys/list/add-service-account-dialog.tsx
Outdated
Show resolved
Hide resolved
web/sdk/react/views/projects/details/project-member-columns.tsx
Outdated
Show resolved
Hide resolved
web/sdk/react/views/projects/details/remove-project-member-dialog.tsx
Outdated
Show resolved
Hide resolved
… RPCs Replace policy-based member management (createPolicyForProject, listPolicies, deletePolicy, createPolicy) with the new atomic RPCs. React SDK: - project-members.tsx: addMember/addTeam use SetProjectMemberRole - project-member-columns.tsx: updateRole uses SetProjectMemberRole - remove-project-member-dialog.tsx: uses RemoveProjectMember - add-service-account-dialog.tsx: uses SetProjectMemberRole - manage-service-user-projects-dialog.tsx: uses both RPCs Admin SDK: - use-add-project-members.tsx: uses SetProjectMemberRole - remove-member.tsx: uses RemoveProjectMember - assign-role.tsx: uses SetProjectMemberRole Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ions Fetch project-scoped roles via listRoles and resolve the role UUID before calling SetProjectMemberRole. Consistent with the role change dropdown which already uses UUIDs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AddMemberDropdown is a separate child component that needs the roles array to resolve the viewer role UUID. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… RemoveProjectMember Update the design-revamped SDK views to use the new atomic RPCs: - add-member-menu.tsx: use SetProjectMemberRole with fetched role UUID - remove-member-dialog.tsx: use RemoveProjectMember - project-details-view.tsx: use SetProjectMemberRole for role change Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix hardcoded principalType in views-new remove-member-dialog: pass memberType through payload to support group removal - Add ownerRoleId to dependency arrays in manage-service-user-projects and add-service-account dialogs to prevent stale closures - Move ownerRoleId validation before service user creation to avoid orphaned service users Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Both views and views-new now pass the member type (user/group) from the column action through to the remove dialog and into the RemoveProjectMember RPC call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SetProjectMemberRole replaces all policies with one role, so the UI should only allow selecting one role. Changed from checkboxes to radio buttons to match the API semantics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Radio component is unstable. Use Checkbox but selecting one automatically deselects others (single-select behavior). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…izationMemberRole Replace listPolicies → deletePolicy × N → createPolicy × N pattern with a single SetOrganizationMemberRole call. Changes multi-role checkboxes to single-role selection since org membership is one role per user. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents silent misuse where group removals would default to 'app/user' principal type and fail. Also fix reset state to include memberType. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit 8b12c65.
- Use isDirty from react-hook-form instead of manual comparison for AssignRole dialogs (org + project), disable Update button when unchanged - Replace hardcoded principal type strings with SCOPES/PRINCIPAL_TYPES constants across admin and react SDK - Early return instead of throwing error when ownerRoleId is missing - Disable create button when ownerRoleId is not available Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/sdk-project-member-management
branch
from
29d8d51 to
8583b0d
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary
createPolicy,deletePolicy,listPolicies) toSetProjectMemberRoleandRemoveProjectMemberRPCs for project member managementSetOrganizationMemberRoleRPC for org member role changesChanges
SDK (React)
SetProjectMemberRole(single atomic call)RemoveProjectMember(single atomic call)AddMemberDropdownreceives roles prop for role selectionAdmin Panel
SetOrganizationMemberRole(single atomic call)listPolicies→deletePolicy× N →createPolicy× N pattern from clientWhy
The old pattern manipulated policies directly from the client — non-atomic, no min-owner checks, error-prone with
Promise.allSettled. The new RPCs handle policy + relation management server-side atomically.Test plan
🤖 Generated with Claude Code