feat(dashboard): URL state synchronization for /settings/root-keys/new route#2218
Conversation
…w route Utilize `nuqs` to establish two-way synchronization between URL query parameters and the page for creating a new root key. The `permissions` query parameter value corresponds to both workspace permissions and any scoped permissions for existing APIs, if available.
…page Addresses a hydration error caused by a `<button>` element being a descendant of another `<button>`. The solution implements the `asChild` prop for Radix UI components, allowing the default rendered element to be replaced with the specified child while merging their props and behaviors.
unrenamed
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 in this pull request focus on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
|
@unrenamed is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
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
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
Line range hint
77-80: Prevent duplicate permissions inselectedPermissionsWhen adding permissions, the current implementation may introduce duplicates if the same permission is added multiple times. To ensure
selectedPermissionscontains unique permissions, consider checking if the permission already exists before adding it.Apply this diff to prevent duplicates:
if (checked) { - return [...prevPermissions, permission]; + return prevPermissions.includes(permission) + ? prevPermissions + : [...prevPermissions, permission]; }
…adix Checkbox API consistency
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (1)
51-58: LGTM: Effective implementation of URL state synchronizationThe switch from
useStatetouseQueryStateforselectedPermissionssuccessfully implements URL state synchronization. The configuration withparseAsArrayOf(parseAsUnkeyPermission)ensures proper handling of permission arrays in the URL.Consider extracting the
useQueryStateoptions into a separate constant for improved readability:const queryStateOptions = { history: "push", shallow: false, clearOnDefault: true, }; const [selectedPermissions, setSelectedPermissions] = useQueryState( "permissions", parseAsArrayOf(parseAsUnkeyPermission).withDefault([]).withOptions(queryStateOptions) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (2 hunks)
🔇 Additional comments (3)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (3)
25-45: LGTM: New imports and parser function align with PR objectivesThe added imports and the new
parseAsUnkeyPermissionfunction are well-implemented to support URL state synchronization for permissions. This change directly addresses the PR objective of enabling two-way synchronization between URL query parameters and the page state.
330-340: LGTM: Improved PermissionToggle structure and accessibilityThe changes to the PermissionToggle component enhance both its structure and accessibility:
- Wrapping the
TooltipTriggerchildren in adivimproves the layout and potential styling of the tooltip trigger area.- Changing the
Checkboxcomponent's prop fromonClicktoonCheckedChangeaddresses the accessibility concern raised in the past review comments.These modifications contribute to a more robust and user-friendly interface.
Line range hint
1-353: LGTM: Successful implementation of URL state synchronization for permissionsThis PR successfully implements the URL state synchronization for the
/settings/root-keys/newroute, addressing the objectives outlined in the PR summary and the linked issue #2147. Key achievements include:
- Implementation of two-way synchronization between URL query parameters and the page state for permissions.
- Utilization of the
nuqslibrary for efficient query state management.- Maintenance of the existing component structure while introducing the new functionality.
- Improved user experience by allowing permissions to be reflected in and loaded from the URL.
The changes are well-implemented and do not introduce any new issues or inconsistencies. This feature will significantly enhance the usability of the root key creation process, allowing users to easily share and reproduce permission configurations.
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
Works great, the only issue is that it's very easy to miss if an api scoped permission was checked cause the card doesn't expand by default
Could you expand all api cards if there's at least one checked permission?
|
While working on this PR, I noticed other places causing rehydration issues, likely due to HTML semantics violations or incorrect use of the Radix UI API (e.g., a inside a ; see TooltipTrigger API). The fix is to use the asChild prop for Radix components triggering these errors. I've fixed this in one place in this PR. But, it might be worth opening a separate issue for this, though it's not critical right now. WDYT, @chronark? |
Good catch! Sure, I'll look into this. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (2)
51-58: LGTM: URL state synchronization for selectedPermissionsThe implementation of
useQueryStateforselectedPermissionssuccessfully achieves the two-way synchronization between URL query parameters and the page state. This aligns well with the PR objectives.However, consider the implications of the
clearOnDefault: trueoption. This might clear the URL when no permissions are selected, which could be confusing for users. Consider setting it tofalseto maintain the URL state even when the default value (empty array) is selected.
97-115: LGTM with suggestion: Efficient initialization of cardStatesMapThe new
useEffecthook efficiently initializescardStatesMapbased on selected permissions. The use of aSetfor O(1) membership checks is a great optimization, reducing the overall complexity to O(n + m) for large data sets.However, the empty dependency array
[]means this effect will only run once on component mount. Consider addingselectedPermissionsandapisto the dependency array to ensurecardStatesMapis updated if these values change:useEffect(() => { // ... existing code ... }, [selectedPermissions, apis]);This will ensure that
cardStatesMapstays in sync with the latest permissions and API list.🧰 Tools
🪛 Biome
[error] 101-101: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (4 hunks)
🧰 Additional context used
🪛 Biome
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx
[error] 101-101: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
🔇 Additional comments (4)
apps/dashboard/app/(app)/settings/root-keys/new/client.tsx (4)
25-25: LGTM: New imports and parser function for URL state synchronizationThe new imports and the
parseAsUnkeyPermissionfunction are well-implemented to support the URL state synchronization feature. The use ofunkeyPermissionValidationensures safe parsing of query values, which aligns with the PR objectives.Also applies to: 28-31, 39-46
Line range hint
80-87: LGTM: Updated handleSetChecked functionThe
handleSetCheckedfunction has been correctly updated to work with the new state management usinguseQueryState. It maintains the expected behavior of adding or removing permissions from theselectedPermissionsarray.
342-352: LGTM: Improved accessibility in PermissionToggle componentThe
PermissionTogglecomponent has been updated to useonCheckedChangeinstead ofonClickfor theCheckboxcomponent. This change improves accessibility and addresses the issue mentioned in the past review comments. The implementation is correct and aligns with best practices for form controls.
Line range hint
1-364: Overall assessment: Well-implemented URL state synchronization featureThe changes in this PR successfully implement the URL state synchronization for the
/settings/root-keys/newroute, meeting the objectives outlined in the PR summary and linked issue #2147. The implementation uses thenuqslibrary effectively, handles permissions correctly, and includes performance optimizations.Key improvements:
- Efficient state management using
useQueryState- Optimized initialization of
cardStatesMap- Enhanced accessibility in the
PermissionTogglecomponentMinor suggestions have been made to further improve the code, including:
- Reconsidering the
clearOnDefaultoption inuseQueryState- Updating the dependency array in the
useEffecthookOverall, this is a solid implementation that enhances user experience and addresses the feature request effectively.
Huh I didn't see this before |
|
The O(1) optimization is probably overkill, outside of some testing workspaces, nobody has more than a handful of apis, but it doesn't hurt I suppose. |
|
Awarding unrenamed: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/unrenamed |
If a malicious user repeats permissions in the URL, the m * n solution could show its downside with exponential growth. The current solution, using a Set for uniqueness, should perform better in such cases. You are right. I seem to be pulling things out of thin air though 😅. If anyone experiences a performance downgrade, it would be the malicious user themselves 😄 |
yep, will do |
2 participants
What does this PR do?
Utilizes
nuqsto establish two-way synchronization between URL query parameters and the page for creating a new root key. Thepermissionsquery parameter value corresponds to both workspace permissions and any scoped permissions for existing APIs, if available.Fixes #2147
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Improvements
TooltipTriggercomponent for better tooltip trigger area structure.