refactor: replace checkAdminOrOwner with team.create permission in teams page#24116
Conversation
…ams page - Replace role-based access control with PBAC using team.create permission - Follow PBAC refactoring guide pattern for server-side permission checking - Use permission-specific variable naming (canCreateTeam) - Maintain existing TeamsListing component interface Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
- Replace isOrgAdmin prop with permissions object containing canCreateTeam - Follow PBAC refactoring guide pattern for UI components - Update TeamsListing component to use permissions.canCreateTeam - Maintain existing functionality while improving code structure - All type checking and linting verification passed Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
the
🧹 Improvements
![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 (3)
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsx (1)
69-71: Use object property shorthand for brevity.Minor readability tweak.
- permissions={{ - canCreateTeam: canCreateTeam, - }} + permissions={{ canCreateTeam }}packages/features/ee/teams/components/TeamsListing.tsx (2)
22-24: Props refactor to permissions object is sound; consider extracting a shared type.Define and reuse a named type to keep server/client in sync.
type TeamsListingProps = { orgId: number | null; - permissions: { - canCreateTeam: boolean; - }; + permissions: TeamsListingPermissions; teams: RouterOutputs["viewer"]["teams"]["list"]; teamNameFromInvite: string | null; errorMsgFromInvite: string | null; }; + +type TeamsListingPermissions = { + canCreateTeam: boolean; +};
56-56: Avoid double-negation and truthy pitfalls with number orgId.Explicit boolean expression reads clearer.
- const isCreateTeamButtonDisabled = !!(orgId && !permissions.canCreateTeam); + const isCreateTeamButtonDisabled = orgId !== null && !permissions.canCreateTeam;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsx(2 hunks)packages/features/ee/teams/components/TeamsListing.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsxpackages/features/ee/teams/components/TeamsListing.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsxpackages/features/ee/teams/components/TeamsListing.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsxpackages/features/ee/teams/components/TeamsListing.tsx
🧬 Code graph analysis (2)
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsx (2)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-306)packages/features/ee/teams/components/TeamsListing.tsx (1)
TeamsListing(30-174)
packages/features/ee/teams/components/TeamsListing.tsx (1)
packages/trpc/react/trpc.ts (1)
RouterOutputs(143-143)
🔇 Additional comments (4)
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsx (2)
76-76: CTA gating aligns with new permission model.Showing CTA when no org or when canCreateTeam is true matches the UI logic downstream.
54-62: Approve PBAC check for team.create. Verified the permission string is used correctly here and no legacy checkAdminOrOwner remains in this file.packages/features/ee/teams/components/TeamsListing.tsx (2)
133-146: Buttons visibility now correctly depends on permissions.canCreateTeam (or no org).This matches server CTA logic and preserves legacy behavior for non-org users.
20-36: All TeamsListing calls match the new prop signature; noisOrgAdminprop remains.
E2E results are ready! |
Udit-takkar
deleted the
devin/1758898927-remove-checkadminorowner-teams
branch
4 participants
What does this PR do?
This PR refactors the teams server page to migrate from role-based access control to Permission-Based Access Control (PBAC), following the established PBAC refactoring guide patterns.
Key Changes:
checkAdminOrOwnerwithPermissionCheckServiceto checkteam.createpermissionTeamsListingcomponent to use apermissionsobject instead ofisOrgAdminboolean propFiles Modified:
apps/web/app/(use-page-wrapper)/(main-nav)/teams/server-page.tsx- Permission check logicpackages/features/ee/teams/components/TeamsListing.tsx- Component props interfaceLink to Devin run: https://app.devin.ai/sessions/24bad72c8df24c66852409ffe4222142
Requested by: @eunjae-lee
How should this be tested?
Test Scenarios:
Test Steps:
/teamspage with different user permission levelsMandatory Tasks
Review Focus Areas
Critical Review Points:
getTeamIdsWithPermissionwithteam.createpermission correctly replacescheckAdminOrOwnerbehavior[MembershipRole.ADMIN, MembershipRole.OWNER]matches original role check logic!orgId || teamIdsWithCreatePermission.includes(orgId))TeamsListingcall sites are broken by the props changeSecurity Considerations:
Checklist