Conversation
WalkthroughThis set of changes introduces comprehensive support for custom team roles alongside traditional roles within the team management features. The main team members page is refactored to use a new Estimated code review effort4 (~90 minutes) Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
✅ No security or compliance issues detected. Reviewed everything up to 3630a01. Security Overview
Detected Code Changes
Reply to this PR with |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
| const facetedTeamValues = { | ||
| roles, | ||
| teams: [team], | ||
| attributes: attributes.map((attribute) => ({ | ||
| id: attribute.id, | ||
| name: attribute.name, | ||
| options: Array.from(new Set(attribute.options.map((option) => option.value))).map((value) => ({ | ||
| value, | ||
| })), | ||
| })), | ||
| }; |
There was a problem hiding this comment.
Populate the filters via server side
dosubot
bot
added
teams
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/21/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (07/24/25)1 label was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (1)
70-80: Filters are already populated server-side.The implementation correctly populates filters server-side using cached functions (
getCachedTeamRolesandgetCachedTeamAttributes). The data is fetched during SSR and passed to the client component.
🧹 Nitpick comments (5)
apps/web/modules/teams/team-members-view.tsx (1)
25-25: Remove unusedattributesprop.The
attributesprop is defined in the interface but never used in the component. Consider removing it to avoid confusion.interface TeamMembersViewProps { team: NonNullable<RouterOutputs["viewer"]["teams"]["get"]>; facetedTeamValues?: { roles: { id: string; name: string }[]; teams: RouterOutputs["viewer"]["teams"]["get"][]; attributes: { id: string; name: string; options: { value: string; }[]; }[]; }; - attributes?: any[]; }packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (1)
82-85: Consider caching the team lookup.Since this handler might be called frequently, consider if the team lookup could be optimized by including
parentIdin the main member query or caching it at a higher level.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (1)
21-47: Consider logging errors for debugging purposes.While returning empty arrays on errors provides good fallback behavior, silently swallowing all errors might make debugging difficult. Consider at least logging errors to help with troubleshooting:
} catch (error) { + console.error(`Failed to fetch team roles for team ${teamId}:`, error); // PBAC not enabled or error occurred, return empty array return []; }Same applies to the
getCachedTeamAttributesfunction.packages/features/pbac/services/role-management.factory.ts (1)
49-49: Document why userId parameter is unused in PBAC implementation.The
_userIdnaming indicates this parameter is unused in PBAC implementation, whileLegacyRoleManagerstill uses it. Consider adding a comment explaining this difference to help future maintainers understand the design decision.packages/features/ee/teams/components/EditMemberSheet.tsx (1)
222-246: Clean implementation of dynamic role selection UI.The conditional rendering between Select and ToggleGroup is well-handled. Consider adding proper TypeScript types instead of using
anyfor the selectedOption parameter:- onChange={(selectedOption: any) => { + onChange={(selectedOption: { value: string; label: string } | null) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx(2 hunks)apps/web/modules/teams/team-members-view.tsx(1 hunks)packages/features/ee/teams/components/EditMemberSheet.tsx(8 hunks)packages/features/ee/teams/components/MemberList.tsx(6 hunks)packages/features/pbac/services/role-management.factory.ts(4 hunks)packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx(3 hunks)packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts(5 hunks)packages/trpc/server/routers/viewer/teams/changeMemberRole.schema.ts(1 hunks)packages/trpc/server/routers/viewer/teams/listMembers.handler.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
apps/web/modules/teams/team-members-view.tsxpackages/trpc/server/routers/viewer/teams/listMembers.handler.tspackages/features/users/components/UserTable/EditSheet/EditUserForm.tsxpackages/trpc/server/routers/viewer/teams/changeMemberRole.schema.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxpackages/features/ee/teams/components/EditMemberSheet.tsxpackages/features/ee/teams/components/MemberList.tsxpackages/features/pbac/services/role-management.factory.tspackages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
🧠 Learnings (3)
packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (3)
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.732Z
Learning: Applies to backend/**/*.{ts,tsx} : For Prisma queries: Only select data you need.
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.732Z
Learning: Applies to backend/**/*.{ts,tsx} : For Prisma queries: Select selects only the fields you specify explicitly.
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.732Z
Learning: Applies to backend/**/*.{ts,tsx} : For Prisma queries: Never use Includes - use select.
packages/features/ee/teams/components/EditMemberSheet.tsx (1)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
packages/features/ee/teams/components/MemberList.tsx (1)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (3)
packages/trpc/server/routers/viewer/teams/changeMemberRole.schema.ts (1)
packages/platform/libraries/index.ts (1)
MembershipRole(98-98)
packages/features/ee/teams/components/MemberList.tsx (1)
packages/features/data-table/lib/utils.ts (1)
convertFacetedValuesToMap(150-154)
packages/features/pbac/services/role-management.factory.ts (3)
packages/platform/libraries/index.ts (1)
MembershipRole(98-98)packages/features/pbac/lib/constants.ts (1)
DEFAULT_ROLE_IDS(7-11)packages/features/pbac/domain/errors/role-management.error.ts (1)
RoleManagementError(1-6)
🪛 Biome (1.9.4)
packages/features/ee/teams/components/MemberList.tsx
[error] 669-672: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 GitHub Check: Tests / Unit
packages/features/pbac/services/role-management.factory.ts
[failure] 62-62: packages/features/pbac/services/tests/role-management.factory.test.ts > RoleManagementFactory > PBACRoleManager > assignRole > should assign default role correctly
RoleManagementError: You do not have access to this role
❯ PBACRoleManager.assignRole packages/features/pbac/services/role-management.factory.ts:62:15
❯ packages/features/pbac/services/tests/role-management.factory.test.ts:132:9
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { code: 'INVALID_ROLE' }
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Security Check
🔇 Additional comments (16)
packages/trpc/server/routers/viewer/teams/changeMemberRole.schema.ts (1)
8-8: LGTM! Schema properly supports custom roles.The union type correctly allows both traditional
MembershipRoleenum values and string identifiers for custom roles, aligning with the PBAC feature requirements.apps/web/modules/teams/team-members-view.tsx (1)
28-64: Well-structured component with proper permission handling.The component correctly:
- Enforces license requirements
- Checks user permissions before displaying member data
- Handles private team visibility appropriately
- Manages modal state cleanly
packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx (1)
110-124: Verify the custom role filtering logic.The current implementation shows only custom roles when
teamRolesexists. This means standard roles (MEMBER, ADMIN, OWNER) won't be available in the dropdown when custom roles are present. Is this the intended behavior?Please confirm if hiding standard roles when custom roles exist is the expected behavior. If users should be able to select both standard and custom roles, consider this approach:
const membershipOptions = useMemo<MembershipOption[]>(() => { const options: MembershipOption[] = []; // Always add standard roles options.push( { value: MembershipRole.MEMBER, label: t("member"), }, { value: MembershipRole.ADMIN, label: t("admin"), } ); if (isOwner) { options.push({ value: MembershipRole.OWNER, label: t("owner"), }); } // Add custom roles if they exist if (teamRoles?.length > 0) { teamRoles.forEach((role) => { options.push({ value: role.id, label: role.name, isCustomRole: true, }); }); } return options; }, [t, isOwner, teamRoles]);packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (1)
81-102: Well-implemented PBAC integration with proper error handling.The implementation correctly:
- Fetches the organization context through team's parentId
- Uses RoleManagementFactory to check PBAC status
- Gracefully handles cases where PBAC is not enabled
- Builds an efficient lookup dictionary for custom roles
packages/features/ee/teams/components/MemberList.tsx (1)
379-402: Excellent implementation of custom role display.The role cell correctly:
- Displays custom role names when available
- Falls back to traditional role names
- Uses the appropriate identifier for filtering
- Maintains backward compatibility
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (5)
1-44: LGTM! Well-structured PBAC integration.The implementation correctly:
- Fetches team information to determine the organizational hierarchy
- Handles the NOT_FOUND case appropriately
- Creates a role manager instance for the correct organization
- Properly transforms permission check errors into TRPC errors
45-56: Consider the implications of dual permission checks.The code performs PBAC permission checks first (lines 36-44) and then traditional permission checks for enum roles. This could create scenarios where:
- PBAC grants permission but traditional checks deny it
- Different error messages for the same operation depending on role type
Consider either:
- Skipping traditional checks when PBAC is enabled
- Documenting this behavior clearly
- Ensuring both systems are aligned in their permission model
68-71: Good addition of null check for target membership.This prevents potential runtime errors and provides clear feedback when attempting to change roles for non-existent members.
86-97: Security consideration: Admin self-elevation via custom roles.The current logic prevents admins from elevating themselves to traditional OWNER role, but this restriction doesn't apply to custom roles. If custom roles can have owner-equivalent permissions, admins could bypass this restriction.
Consider whether custom roles should also be subject to self-elevation restrictions based on their permission levels rather than just their type.
99-121: LGTM! Clean role assignment implementation.The code properly delegates to the role manager and handles errors appropriately. Note that the
userIdparameter passed toassignRoleappears to be unused in the PBAC implementation (based on the parameter being renamed to_userIdin PBACRoleManager).apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (1)
49-69: Well-structured data fetching with proper error handling.The implementation correctly:
- Handles async params (Next.js 15 pattern)
- Validates team existence before proceeding
- Efficiently fetches data in parallel
- Properly determines the organization ID
packages/features/pbac/services/role-management.factory.ts (1)
21-22: Clean implementation of getTeamRoles method.The method is well-designed with:
- Consistent interface across both managers
- Appropriate delegation in PBAC implementation
- Sensible defaults in legacy implementation
- Consistent return type structure
Also applies to: 79-86, 129-136
packages/features/ee/teams/components/EditMemberSheet.tsx (4)
27-28: Good schema evolution for role flexibility.The union type properly supports both traditional enum roles and custom string role IDs, maintaining backward compatibility while enabling new functionality.
50-50: Well-implemented custom roles integration.The implementation correctly:
- Initializes state with custom role ID when available
- Configures the query to avoid retries when PBAC is disabled
- Only enables the query when teamId is present
Also applies to: 64-71
72-101: Consider the implications of exclusive role display.The current implementation shows either custom roles OR traditional roles, but never both. This means:
- Organizations with custom roles lose access to traditional role options in the UI
- There's no gradual migration path from traditional to custom roles
Is this intentional? Consider whether organizations might want to use both traditional and custom roles during a transition period.
105-105: Consistent handling of role data throughout the component.The implementation properly:
- Initializes form values with custom role support
- Maintains type consistency by casting to string
- Combines loading states to ensure data is ready before rendering
Also applies to: 132-132, 178-178
| const allRoles = facetedTeamValues.roles.map((role) => ({ | ||
| label: role.name, | ||
| value: role.id, | ||
| })); |
There was a problem hiding this comment.
Fix switch declaration scope issue.
Variables declared in switch cases without block scope can be accessed by other cases, which can lead to bugs.
case "role":
+ {
// Include both traditional roles and PBAC custom roles
const allRoles = facetedTeamValues.roles.map((role) => ({
label: role.name,
value: role.id,
}));
return convertFacetedValuesToMap(allRoles);
+ }🧰 Tools
🪛 Biome (1.9.4)
[error] 669-672: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In packages/features/ee/teams/components/MemberList.tsx around lines 669 to 672,
the variables declared inside the switch cases lack block scope, which can cause
them to be accessed by other cases unintentionally. To fix this, wrap the code
inside each switch case in curly braces {} to create a block scope, ensuring
variables are scoped only within their respective cases.
packages/features/users/components/UserTable/EditSheet/EditUserForm.tsx
Outdated
Show resolved
Hide resolved
sean-brydon
force-pushed
the
fix/assigning-child-team-custom-roles
branch
from
042bf61 to
221d3a2
Compare
eunjae-lee
left a comment
eunjae-lee
left a comment
There was a problem hiding this comment.
I tested it and it's working well!
I see some type errors and NITs from CodeRabbit.
I'll revisit this !
- Add missing onSettingsOpen prop to MemberInvitationModalWithoutMembers - Update updateRoleInCache function to handle both MembershipRole enum and custom role strings - Fix cache update logic to properly handle mixed role types without breaking type safety - Ensure proper type handling for both traditional and custom roles in EditMemberSheet Co-Authored-By: sean@cal.com <Sean@brydon.io>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/modules/teams/team-members-view.tsx(1 hunks)packages/features/ee/teams/components/EditMemberSheet.tsx(8 hunks)packages/features/ee/teams/components/MemberChangeRoleModal.tsx(2 hunks)
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/features/ee/teams/components/MemberChangeRoleModal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/modules/teams/team-members-view.tsx
- packages/features/ee/teams/components/EditMemberSheet.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/features/ee/teams/components/MemberChangeRoleModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/features/ee/teams/components/MemberChangeRoleModal.tsx (1)
27-27: LGTM: Type expansion supports custom roles.The expansion of the
roleparameter type fromMembershipRoletoMembershipRole | stringcorrectly enables support for both traditional enum roles and custom role IDs (strings), aligning with the PBAC feature requirements.
- Fix assignRole method to properly check if role is a default MembershipRole enum key - Use DEFAULT_ROLE_IDS mapping to get the correct role ID for default roles - Resolves unit test failure in role-management.factory.test.ts Co-Authored-By: sean@cal.com <Sean@brydon.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (1)
84-95: Prevent ADMIN users from self-elevating to custom PBAC rolesThe current guard in changeMemberRole.handler.ts only blocks self-elevation when
input.roleis one of the defaultMembershipRoleenum values. Custom role IDs (strings) bypass this check, allowing an ADMIN to assign themselves arbitrarily powerful PBAC roles.Please add a complementary check for custom roles—e.g. before calling
roleManager.assignRole(...), insert:if ( myMembership?.role === MembershipRole.ADMIN && input.memberId === ctx.user.id && typeof input.role === "string" && - Object.values(MembershipRole).includes(input.role as MembershipRole) && - input.role !== MembershipRole.MEMBER + ( + // default enums except MEMBER + (Object.values(MembershipRole).includes(input.role as MembershipRole) && input.role !== MembershipRole.MEMBER) + // OR any custom role string + || !Object.values(MembershipRole).includes(input.role as MembershipRole) + ) ) { throw new TRPCError({ code: "FORBIDDEN", message: "You can not change yourself to a higher role.", }); }This ensures ADMINs cannot elevate themselves to neither higher default roles nor custom PBAC roles.
♻️ Duplicate comments (1)
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (1)
1-3: LGTM: Repository pattern implementation addresses previous feedback.The introduction of
TeamRepositoryandRoleManagementFactoryimproves the architecture by implementing the repository pattern as suggested in the previous review comment.
🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (2)
34-41: Enhance error handling specificity.The generic error handling could be improved to provide more specific error messages based on the type of permission failure.
Consider enhancing the error handling:
try { await roleManager.checkPermissionToChangeRole(ctx.user.id, organizationId); } catch (error) { + const errorMessage = error instanceof Error ? error.message : "Unauthorized"; + // Log the specific error for debugging while keeping user message generic + console.error('Permission check failed:', error); throw new TRPCError({ code: "UNAUTHORIZED", - message: error instanceof Error ? error.message : "Unauthorized", + message: errorMessage, }); }
97-106: Improve error context for role assignment failures.The generic "Failed to assign role" message could be more helpful for debugging and user understanding.
Consider providing more context in the error message:
try { await roleManager.assignRole(input.memberId, input.teamId, input.role, targetMembership.id); } catch (error) { + const errorMessage = error instanceof Error ? error.message : "Failed to assign role"; + // Log the full error for debugging + console.error('Role assignment failed:', { + memberId: input.memberId, + teamId: input.teamId, + role: input.role, + error + }); throw new TRPCError({ code: "BAD_REQUEST", - message: error instanceof Error ? error.message : "Failed to assign role", + message: errorMessage, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/features/ee/teams/components/EditMemberSheet.tsx(8 hunks)packages/features/ee/teams/components/MemberChangeRoleModal.tsx(2 hunks)packages/features/pbac/services/role-management.factory.ts(4 hunks)packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts(5 hunks)packages/trpc/server/routers/viewer/teams/listMembers.handler.ts(3 hunks)
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/features/ee/teams/components/MemberChangeRoleModal.tsx
- packages/trpc/server/routers/viewer/teams/listMembers.handler.ts
- packages/features/ee/teams/components/EditMemberSheet.tsx
- packages/features/pbac/services/role-management.factory.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (3)
packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts (3)
66-68: LGTM: Improved error handling for missing membership.The explicit check for target membership with a specific error message improves the user experience compared to potential null reference errors later.
108-118: I couldn’t find any separate “Role” model or relation in your Prisma schema—your Membership model already defines a scalar role field (and Prisma returns all scalar fields by default). Including the role column explicitly isn’t necessary, and there’s no extra table to join. You can safely ignore this suggestion.Likely an incorrect or invalid review comment.
20-31: Team repository error handling and organization ID logic are correctly implemented
TeamRepository.findByIdreturnsnullwhen no matching team exists, which the handler immediately converts to aNOT_FOUNDerror. The expressionteam.parentId || input.teamIdaccurately yields the organization’s ID for nested teams and the team’s own ID for organization or standalone teams. Since we only callcreateRoleManagerwith a validated ID, no further checks are necessary.
…-team-custom-roles' into fix/assigning-child-team-custom-roles
Co-authored-by: sean@cal.com <Sean@brydon.io> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
4 participants
What does this PR do?
This PR adds the ability for people to assign sub-team roles to members in the team when pbac is enabled.
Visual Demo (For contributors especially)
Image Demo (if applicable):
How to test:
Enable PBAC on a org by adding pbac to the team features for that specific team in DB.
Manually refresh the app to invalidate team features cache.
Visit roles and permissions in settings sidebar fo teams. Create a role with any permissions - youll want to check the role CRUD here.
Goto the team members page - check filters work
Edit a user - move them to the new role you created
impersonate
Ensure you can edit other users
Filter by new role
logout
login to teampro or any other account with pbac disabled
Ensure filters and chaning role works as expected