fix: show shallow permissions followed by nested in alphabetical order#2273
Conversation
DeepaPrasanna
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes made in this pull request introduce a new sorting mechanism for permissions within the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
@DeepaPrasanna is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
🕹️ 150 points
Bug
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (3)
46-55: LGTM: Sorting logic implementation is correct and aligns with PR objectives.The sorting mechanism effectively ensures that shallow permissions (those with fewer parts in their names) appear before nested ones, and then sorts them alphabetically. This implementation correctly addresses the issue #1011 and meets the PR objectives.
58-58: LGTM: Correct usage of sorted permissions in the loop.The
forloop now correctly iterates over thesortedPermissionsarray, ensuring that thenestedobject is constructed in the desired order (shallow permissions first, then nested, all alphabetically sorted within their groups).
46-58: Summary: Changes successfully implement required permission sorting.The introduced changes effectively address the issue #1011 and meet the PR objectives. The sorting mechanism ensures that shallow permissions are displayed before nested ones, with both groups sorted alphabetically. The implementation is well-integrated into the existing component structure and doesn't introduce unnecessary complexity.
Key points:
- Correct sorting logic implementation (lines 46-55).
- Proper usage of sorted permissions in the nested object construction (line 58).
- Minimal impact on the overall component structure.
The changes look good to merge, with the optional consideration of the previously suggested performance optimization if needed in the future.
|
Found out that the long permission text overflows |
|
Is this good? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (2)
17-43: Excellent implementation of permission sorting!The
sortNestedPermissionsfunction effectively separates shallow and nested permissions, sorts them alphabetically, and combines them in the required order. This aligns perfectly with the PR objectives and the requirements from the linked issue #1011.Consider a minor optimization:
function sortNestedPermissions(nested: NestedPermissions) { const [shallowPermissions, nestedPermissions] = Object.entries(nested).reduce( ([shallow, nested], [key, value]) => { (Object.keys(value.permissions).length > 0 ? nested : shallow)[key] = value; return [shallow, nested]; }, [{} as NestedPermissions, {} as NestedPermissions] ); return { ...Object.fromEntries(Object.entries(shallowPermissions).sort(([a], [b]) => a.localeCompare(b))), ...Object.fromEntries(Object.entries(nestedPermissions).sort(([a], [b]) => a.localeCompare(b))) }; }This version reduces the number of iterations and uses more functional programming concepts, which might be more concise and potentially more performant for larger permission sets.
74-83: Well-implemented sorting logic for permissions!The sorting mechanism effectively ensures that shallow permissions appear before nested ones, with both categories sorted alphabetically. This implementation aligns perfectly with the PR objectives and the requirements from the linked issue #1011.
For improved clarity, consider extracting the sorting logic into a separate function:
const sortPermissions = (a: Permission, b: Permission) => { const aParts = a.name.split("."); const bParts = b.name.split("."); if (aParts.length !== bParts.length) { return aParts.length - bParts.length; } return a.name.localeCompare(b.name); }; const sortedPermissions = workspace.permissions.sort(sortPermissions);This separation of concerns makes the code more readable and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (1)
86-86: Excellent integration of the new sorting logic!The changes effectively incorporate the new sorting mechanisms into the existing component structure:
- Using
sortedPermissionsto build the nested structure ensures that permissions are initially in the correct order.- Applying
sortNestedPermissionsto thenestedobject further refines the ordering.- Passing
sortedNestedPermissionsto theTreecomponent ensures that the UI reflects the desired permission order.These changes successfully implement the requirements from the linked issue #1011 and meet the PR objectives.
Also applies to: 106-107, 125-125
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (2)
17-43: LGTM! Consider a minor optimization for object creation.The
sortNestedPermissionsfunction effectively implements the requirement to show shallow permissions before nested ones, both sorted alphabetically. The logic is clear and well-structured.For a minor optimization, consider using
Object.fromEntries()when creating the final sorted object:const sortedObject: NestedPermissions = Object.fromEntries([ ...sortedShallowKeys.map(key => [key, shallowPermissions[key]]), ...sortedNestedKeys.map(key => [key, nestedPermissions[key]]) ]);This approach creates the object in a single step, potentially improving performance for large permission sets.
74-83: LGTM! Consider extracting the comparison logic for better readability.The sorting logic correctly implements the requirement to show shallow permissions before nested ones, with alphabetical ordering within each group. The approach is effective and locale-aware.
For improved readability, consider extracting the comparison logic into a separate function:
const comparePermissions = (a, b) => { const aParts = a.name.split("."); const bParts = b.name.split("."); if (aParts.length !== bParts.length) { return aParts.length - bParts.length; } return a.name.localeCompare(b.name); }; const sortedPermissions = workspace.permissions.sort(comparePermissions);This extraction would make the sorting logic more self-documenting and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (1)
86-86: LGTM! The changes correctly implement the new sorting requirements.The modifications effectively integrate the new sorting logic into the component:
- Using
sortedPermissionsto build thenestedobject ensures correct initial ordering.- Applying
sortNestedPermissionstonestedfurther refines the order.- Passing
sortedNestedPermissionsto theTreecomponent ensures the UI reflects the new sorting.These changes successfully fulfill the PR objectives of showing shallow permissions before nested ones, both in alphabetical order.
To ensure the changes are correctly applied throughout the codebase, let's verify the usage of the
Treecomponent:This will help confirm that the sorting changes are consistently applied wherever the
Treecomponent is used.Also applies to: 106-106, 125-125
|
Hello @chronark, it seems even if you enabled auto merge few days ago, this wasn't merged. Is there anything else I could do with respect to this issue? |
|
Awarding DeepaPrasanna: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/DeepaPrasanna |
unkeyed#2273) * fix: show shallow permissions followed by nested in alphabetical order * fix: correct the sorting of nested permissions top level keys * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* fix: Missing plan check and ip whitelist parsing * fix: adjust tests for ipwhitelist * fix: Rename error code Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: ipwhitelist via features vs enterprise plan * fix: invert condition * chore: add cache log * fix: ensure workspace is loaded (#2470) * chore: more logging and retries (#2475) * Update 7_create_a_template.md (#2471) * increase override limit * Update create-new-override.tsx * fix: Enhance API Key Detail Page: Change Permissions Visualization to Tree Format (#2238) * fix:changed the permission view * fixed issue comments * [autofix.ci] apply automated fixes * removed font --------- Co-authored-by: Andreas Thomas <dev@chronark.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * fix: show shallow permissions followed by nested in alphabetical order (#2273) * fix: show shallow permissions followed by nested in alphabetical order * fix: correct the sorting of nested permissions top level keys * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * Add Template to Markdown (#2362) Co-authored-by: Andreas Thomas <dev@chronark.com> * fix: retry on any error with disabled cache --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: chronark <dev@chronark.com> Co-authored-by: Chirag Arora <chirag8023@gmail.com> Co-authored-by: RajuGangitla <gangitlaraju8520@gmail.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Anne Deepa Prasanna <anneraj73@gmail.com> Co-authored-by: djnovin <95838226+djnovin@users.noreply.github.com>
2 participants
What does this PR do?
Fixes #1011
Before:
After:
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes