chore: add permissions.canUpdateOrganization to settings#24142
Conversation
WalkthroughThis change introduces a new permission flag, canUpdateOrganization, into the settings permissions flow. The server-side layout computes canUpdateOrganization from organization permissions (CrudAction.Update) with a fallback to isOrgAdminOrOwner and passes it to SettingsLayoutAppDirClient. The client updates tab-filtering logic for the organizations section to conditionally show attributes, billing, developer (admin_api), and other_teams tabs based on permissions?.canUpdateOrganization in addition to isOrgAdminOrOwner. Dependency arrays in useTabs are updated to include permissions. The SettingsPermissions interface is expanded to include canUpdateOrganization. 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 |
dosubot
bot
added
the
🧹 Improvements
sean-brydon
added
ready-for-e2e
and removed
🧹 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
220-226: Delegation-credential menu bypasses permission gateYou push "delegation_credential" after filtering by organizationAdminKeys, so it will show for users without canUpdateOrganization/isOrgAdminOrOwner whenever the feature flag is on. Gate it consistently.
Apply this diff:
- // Add delegation-credential menu item only if feature flag is enabled - if (isDelegationCredentialEnabled) { + // Add delegation-credential only if feature flag AND user can update org + if (isDelegationCredentialEnabled && (permissions?.canUpdateOrganization || isOrgAdminOrOwner)) { newArray.push({ name: "delegation_credential", href: "/settings/organizations/delegation-credential", }); }apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx (1)
20-29: Bug: unstable_cache key does not include teamIdgetTeamFeatures caches by a static key; results for different teamId values can collide. Include teamId in the key.
-const getTeamFeatures = unstable_cache( - async (teamId: number) => { +const getTeamFeatures = (teamId: number) => + unstable_cache( + async () => { const featuresRepository = new FeaturesRepository(prisma); - return await featuresRepository.getTeamFeatures(teamId); - }, - ["team-features"], - { - revalidate: 120, - } -); + return await featuresRepository.getTeamFeatures(teamId); + }, + ["team-features", String(teamId)], + { revalidate: 120 } + )();
🧹 Nitpick comments (3)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (3)
213-218: Avoid brittle index when inserting "attributes"Hard-coding splice(4, 0, ...) risks ordering bugs if the child list changes. Insert relative to an anchor key instead.
Apply this diff:
- if (permissions?.canUpdateOrganization || isOrgAdminOrOwner) { - newArray.splice(4, 0, { - name: "attributes", - href: "/settings/organizations/attributes", - }); - } + if (permissions?.canUpdateOrganization || isOrgAdminOrOwner) { + const after = newArray.findIndex((c) => c.name === "privacy"); + const insertAt = after >= 0 ? after + 1 : newArray.length; + newArray.splice(insertAt, 0, { + name: "attributes", + href: "/settings/organizations/attributes", + }); + }
270-274: Clarify the admin_api filter predicateSame behavior, but easier to read when the name check comes first.
- const filtered = tab?.children?.filter( - (childTab) => - permissions?.canUpdateOrganization || isOrgAdminOrOwner || childTab.name !== "admin_api" - ); + const filtered = tab?.children?.filter( + (childTab) => childTab.name !== "admin_api" || permissions?.canUpdateOrganization || isOrgAdminOrOwner + );
288-296: Memo deps updated appropriatelyIncluding permissions in deps is necessary so tab visibility reacts to permission changes. If unwanted recomputes arise, consider depending on specific booleans instead of the whole object.
📜 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)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx(6 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.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)/settings/(settings-layout)/layout.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.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)/settings/(settings-layout)/layout.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.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)/settings/(settings-layout)/layout.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx
🧬 Code graph analysis (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx (1)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (1)
isOrgAdminOrOwner(38-47)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
apps/api/v2/src/modules/organizations/memberships/services/organizations-membership.service.ts (1)
isOrgAdminOrOwner(38-47)
⏰ Context from checks skipped due to timeout of 180000ms. 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 (9)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (5)
175-179: Type addition looks goodAdding canUpdateOrganization to SettingsPermissions is correct and aligns with usage below.
206-211: Org tab filtering: condition reads correctlyIncluding permissions?.canUpdateOrganization alongside isOrgAdminOrOwner is the right gate for admin-only children.
245-251: Billing visibility (non-PBAC) matches intentShowing org billing when canUpdateOrganization || isOrgAdminOrOwner (outside PBAC) is consistent with the new flag.
280-287: Gating "other_teams" with canUpdateOrganizationHiding the section unless canUpdateOrganization || isOrgAdminOrOwner aligns with PBAC expectations.
659-669: "Add a team" visibility aligns with new permissionAllowing creation when canUpdateOrganization || isOrgAdminOrOwner (or no org set) makes sense.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/layout.tsx (4)
50-50: Init canUpdateOrganization defaultDefaulting to false is correct.
72-72: PBAC mapping for canUpdateOrganization is correctUsing CrudAction.Update with fallback to isOrgAdminOrOwner matches the PR objective.
81-82: Passing canUpdateOrganization to clientPlumbing the flag through the permissions object looks good.
31-38: Ignore incorrect static key concern
unstable_cacheautomatically incorporates its function arguments (and function source) into the cache key; supplying["resource-permissions"]merely adds to, not replaces, that key. No changes required.Likely an incorrect or invalid review comment.
E2E results are ready! |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
4 participants
What does this PR do?
This PR adds the logic to ensure canUpdate shows/hides the correct navigation items in settings layout
CleanShot.2025-09-29.at.13.24.39.mp4
How should this be tested?
Create a org
Enable PBAC
Create a new role
Assign a member this role
View settings -> notice you cant see any of the UI elements to edit org
Toggle permissions to have ALL or specific permissions on orgs.
Hard refresh other window -> profit