fix: disable save button when workspace name is empty #2294
Conversation
RajuGangitla
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance role and permission management within the API key detail page, introduce a new 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 (
|
|
@RajuGangitla 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: 8
🧹 Outside diff range and nitpick comments (7)
apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (1)
21-24: Approve changes with a minor correctionThe updates to the
formSchemafor thenamefield are good:
- The minimum length requirement of 3 characters is appropriate.
- Using
refinewithvalidCharactersRegexensures proper character validation.However, there's a small typo in the error message:
- message: "worksapce can only contain letters, numbers, dashes and underscores", + message: "workspace can only contain letters, numbers, dashes and underscores",apps/dashboard/app/(app)/authorization/roles/[roleId]/tree.tsx (1)
59-59: Approved: RecursivePermission component is now exportedThe change to export the
RecursivePermissioncomponent is appropriate and aligns with potential reuse in other parts of the application, such as workspace name validation mentioned in the PR objectives.Consider updating the component's documentation (if any) to reflect its new exportable status and potential usage in other modules.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx (2)
43-45: Optimize dependency array in useEffectIncluding
rolesin the dependency array of theuseEffecthook may cause unnecessary re-renders if therolesarray reference changes, even when its contents are the same. Ifrolesdoes not change frequently or is already memoized upstream, consider removing it from the dependency array to prevent potential performance issues.
55-55: Addaria-labeltoSwitchcomponent for accessibilityConsider adding an
aria-labelto theSwitchcomponent to improve accessibility for users utilizing screen readers.Apply this change:
<Switch checked={openAll} onCheckedChange={setOpenAll} + aria-label="Expand or Collapse All Roles" />packages/api/src/openapi.d.ts (3)
599-613: Clarify default behaviors for optionallimitanddurationinratelimitsThe
limitanddurationproperties are optional. It's important to specify how the system behaves when these are undefined. Consider documenting default values or making these fields required if they're essential for ratelimiting functionality.
Line range hint
1296-1328: Typographical error inasyncproperty descriptionThere's a typo in the description of the
asyncproperty: "Asnyc" should be "Async."Apply this diff to correct the typo:
- * @description Asnyc ratelimiting doesn't add latency, while sync ratelimiting is slightly more accurate. + * @description Async ratelimiting doesn't add latency, while sync ratelimiting is slightly more accurate.
Line range hint
2585-2587: Typographical error inmetadescriptionThe word "resources" should be singular in the description.
Apply this diff to correct the description:
- /** @description Attach any metadata to this resources */ + /** @description Attach any metadata to this resource */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx (6 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx (1 hunks)
- apps/dashboard/app/(app)/authorization/roles/[roleId]/tree.tsx (1 hunks)
- apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (3 hunks)
- apps/dashboard/app/layout.tsx (1 hunks)
- apps/engineering/app/layout.tsx (1 hunks)
- apps/planetfall/app/layout.tsx (1 hunks)
- apps/workflows/app/layout.tsx (1 hunks)
- packages/api/src/openapi.d.ts (34 hunks)
🧰 Additional context used
🪛 Biome
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx
[error] 28-29: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
[error] 185-185: This let declares a variable that is only assigned once.
'data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (14)
apps/workflows/app/layout.tsx (1)
4-8: Approved font optimization changes, but clarification needed.The changes to the Inter font configuration look good and can potentially improve the font loading experience:
display: 'swap'will show a fallback font until Inter is loaded, improving perceived performance.adjustFontFallback: falsedisables Next.js's automatic font fallback adjustments, which might be intentional for manual control.These optimizations are beneficial, but they don't seem directly related to the PR's stated objective of disabling the save button when the workspace name is empty. Could you please clarify how these font changes contribute to the PR's main goal?
To ensure these changes don't negatively impact other parts of the application, please run the following command to check for any other font configurations that might need similar updates:
✅ Verification successful
Font configuration changes in
apps/workflows/app/layout.tsxare isolated and verified.No other font configurations were found in the project, ensuring that the changes are limited to this file and do not impact other parts of the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other font configurations in the project # Expected result: A list of files with font configurations, if any rg --type typescript --type javascript 'const.*=.*next/font/.*\(' -g '!**/layout.tsx'Length of output: 122
Script:
#!/bin/bash # Description: Check for other font configurations in the project with corrected file types # Expected result: A list of files with font configurations, if any rg --type ts --type tsx --type js --type jsx 'const.*=.*next/font/.*\(' -g '!**/layout.tsx'Length of output: 121
Script:
#!/bin/bash # Description: Check for other font configurations in the project by searching file extensions directly # Expected result: A list of files with font configurations, if any rg 'const\s*=\s*next/font/\w+\(' --glob '!**/layout.tsx' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx'Length of output: 116
apps/planetfall/app/layout.tsx (2)
Line range hint
1-31: Clarification needed on PR scope and completeness.The changes in this file, which modify the font configuration, do not appear to be related to the stated PR objective of "disabling the save button when the workspace name is empty" (issue #2260).
To ensure we're not missing any relevant changes, please run the following commands:
#!/bin/bash # Description: Search for files related to workspace name validation or save button functionality # Test 1: Look for files containing workspace name related code echo "Searching for workspace name related files:" rg --type typescript --type javascript "workspace.?name" # Test 2: Look for files containing save button related code echo "Searching for save button related files:" rg --type typescript --type javascript "save.?button|disable.?save" # Test 3: Check for recent commits related to the PR objective echo "Checking recent commits:" git log --grep="workspace name\|save button" --since="1 week ago" --onelineCould you please clarify if there are additional files that should be included in this review to address the PR objective? If this font configuration change is intended to be part of this PR, could you explain its relevance to the stated objective?
5-9: Font configuration changes may impact visual stability.The modifications to the
Interfont configuration introduce two new properties:
display: 'swap': This will show text immediately using a fallback font while loading the custom font, potentially causing a noticeable font change (FOUT).adjustFontFallback: false: This disables Next.js's automatic adjustment to reduce layout shift, which might lead to more noticeable layout shifts when the custom font loads.These changes prioritize faster initial text rendering over reducing layout shift. While this could be a performance optimization, it may affect the visual stability of the page.
Please verify the visual impact of these changes across different devices and network conditions. Consider running the following commands to check for potential regressions:
Consider adding a comment explaining the reasoning behind these configuration changes to help future maintainers understand the trade-offs made.
apps/engineering/app/layout.tsx (1)
9-10: LGTM! Consider monitoring the impact on user experience.The changes to the Inter font configuration look good and align with common web font loading strategies:
display: 'swap'can improve perceived loading performance by showing content quickly.adjustFontFallback: falsegives more control over font rendering.However, these changes might impact the user experience:
- The 'swap' strategy may cause a noticeable font change (FOUT - Flash of Unstyled Text).
- Disabling
adjustFontFallbackmight lead to more noticeable layout shifts when the font loads.To ensure these changes don't negatively impact the user experience, consider running the following checks:
These checks will help identify any potential conflicts or areas that might need adjustment based on the new font configuration.
✅ Verification successful
LGTM! The changes to the Inter font configuration have been verified and align with standard web font loading strategies.
- No other font configurations were found that might conflict with these changes.
- Existing performance monitoring tools are in place to track any impact on user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other font configurations in the project # Test 1: Check if there are other font configurations that might conflict echo "Checking for other font configurations:" rg --type typescript --type javascript 'new\s+NextFont' -g '!**/layout.tsx' # Test 2: Check if there are any CSS rules that might affect font rendering echo "Checking for CSS rules affecting font rendering:" rg --type css 'font-display|@font-face' # Test 3: Check if there are any performance monitoring tools in place echo "Checking for performance monitoring setup:" rg --type typescript --type javascript 'performance|webVitals'Length of output: 661
Script:
#!/bin/bash # Description: Check for other font configurations in the project # Test 1: Check if there are other font configurations that might conflict echo "Checking for other font configurations:" rg --type ts --type js 'new\s+NextFont' -g '!**/layout.tsx' # Test 2: Check if there are any CSS rules that might affect font rendering echo "Checking for CSS rules affecting font rendering:" rg --type css 'font-display|@font-face' # Test 3: Check if there are any performance monitoring tools in place echo "Checking for performance monitoring setup:" rg --type ts --type js 'performance|webVitals'Length of output: 3992
apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (4)
17-17: LGTM: Appropriate regex for workspace name validationThe
validCharactersRegexconstant is well-defined to allow letters, numbers, dashes, and underscores in the workspace name. This aligns with common naming conventions and the PR objectives.
62-62: LGTM: Proper disabled state logicThe
isDisabledconstant correctly combines the form's loading state and validity state. This ensures that the save button will be disabled not only during form submission but also when the input is invalid, which aligns perfectly with the PR objectives.
93-93: LGTM: Correct implementation of button disabled stateThe update to the submit button's
disabledproperty correctly implements the PR objective. By using bothupdateName.isLoadingand theisDisabledconstant, the button will be disabled when:
- The form is being submitted
- The form is invalid (including when the workspace name is empty)
This change enhances the user experience by preventing submissions with invalid data.
Line range hint
1-101: Summary: PR objectives successfully implemented with minor improvement neededThe changes in this file successfully implement the PR objectives:
- The save button is now disabled when the workspace name is empty.
- Additional validation has been added to ensure the workspace name meets specific criteria.
These improvements enhance the user experience and prevent submission of invalid data. The implementation is clean and aligns well with React and form handling best practices.
One minor improvement is needed:
- Fix the typo in the error message for invalid workspace names.
Overall, great job on implementing these features!
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx (1)
14-14: [Approved] ImportingRolefrom@/lib/dbThe addition of
Rolein the import statement is appropriate for the enhanced role management functionality.packages/api/src/openapi.d.ts (5)
9-10: Type definitions forXORandOneOfare correctly implementedThe updated type definitions enhance type safety and are well-structured for representing exclusive and one-of type constraints.
524-524: Update tocodeproperty inV1KeysVerifyKeyResponseis appropriateDefining the
codeproperty as a union of string literals clearly enumerates all possible status codes, improving type correctness and readability.
550-554:PermissionQuerytype definition is clear and recursiveThe use of
OneOfand the recursive structure allows for complex permission queries combining strings with logicaland/oroperators.
1341-1349:refillproperty definition is precise and clearThe
refilloption is well-defined, allowing for automatic refill settings with clear interval choices.
1558-1579: Addition of detailed usage metrics inverificationsIncluding properties like
success,rateLimited, andusageExceededprovides valuable insights for monitoring and analytics.
apps/dashboard/app/layout.tsx
Outdated
| display: 'swap', | ||
| adjustFontFallback: false |
There was a problem hiding this comment.
💡 Codebase verification
Remove or correct the adjustFontFallback property.
The adjustFontFallback: false property does not appear to be defined or implemented anywhere in the codebase. It is likely not a valid configuration option for the Inter font in next/font/google. Please remove this property to prevent potential issues or unintended behavior.
🔗 Analysis chain
Approve font swap, but verify adjustFontFallback property.
The addition of display: 'swap' is a good practice. It improves user experience by showing a fallback font while the custom font is loading, preventing invisible text.
However, adjustFontFallback: false is not a standard option for the next/font/google Inter font configuration. Please verify if this is a custom property or if it's meant to be something else.
To check if adjustFontFallback is a valid option, let's search for its usage in the project:
Verify the implementation of adjustFontFallback across the codebase.
The property adjustFontFallback: false appears consistently in multiple layout.tsx files, indicating it may be a custom configuration specific to this project.
To ensure it's correctly implemented and intended, please verify where and how adjustFontFallback is defined and utilized within the codebase.
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for 'adjustFontFallback' in all TypeScript and JavaScript files
rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' --type ts --type js 'adjustFontFallback'
Length of output: 332
Script:
#!/bin/bash
# Search for the definition or implementation of 'adjustFontFallback'
rg 'adjustFontFallback' --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' .
Length of output: 322
| {Object.entries(role.nestedPermissions).map(([k, p]) => ( | ||
| <RecursivePermission | ||
| key={p.id} | ||
| k={k} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename prop k to permissionKey for clarity
In the RecursivePermission component, consider renaming the prop k to permissionKey to improve readability and adhere to naming conventions.
Apply this diff:
- k={k}
+ permissionKey={k}Ensure that the RecursivePermission component is updated accordingly to accept the new permissionKey prop.
Committable suggestion was skipped due to low confidence.
| </TooltipTrigger> | ||
| <TooltipContent side="top" align="start" avoidCollisions={true}> | ||
| <div className="flex items-center justify-start max-w-sm gap-2 text-content"> | ||
| <span className="text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</span> |
There was a problem hiding this comment.
Ensure overflow behavior works correctly on inline elements
The overflow-hidden and hover:overflow-visible classes may not work as expected on a span element because it is an inline element. To ensure the overflow behavior functions correctly, consider changing the span to a div or adding inline-block to the class name.
Apply one of the following changes:
Option 1: Change span to div
-<span className="text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</span>
+<div className="text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</div>Option 2: Add inline-block to the class name
<span
+ className="inline-block text-ellipsis overflow-hidden hover:overflow-visible"
>
{role.name}
</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span className="text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</span> | |
| <div className="text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</div> | |
| ``` | |
| Option 2: Add `inline-block` to the class name | |
| ```suggestion | |
| <span className="inline-block text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</span> |
| @@ -155,6 +160,41 @@ export default async function APIKeyDetailPage(props: { | |||
| } | |||
| } | |||
|
|
|||
| const roleTee = key.workspace.roles.map((role) => { | |||
There was a problem hiding this comment.
Correct variable name from roleTee to roleTree
The variable roleTee on line 163 appears to be a typo. It should likely be roleTree to reflect that it represents a tree structure of roles. This change should also be made wherever roleTee is used, such as on line 351.
Apply this diff to fix the typo:
- const roleTee = key.workspace.roles.map((role) => {
+ const roleTree = key.workspace.roles.map((role) => {And update the usage on line 351:
-<PermissionTree roles={roleTee} />
+<PermissionTree roles={roleTree} />Also applies to: 351-351
| @@ -25,6 +25,9 @@ import ms from "ms"; | |||
| import { notFound } from "next/navigation"; | |||
| import { Chart } from "./chart"; | |||
| import { VerificationTable } from "./verification-table"; | |||
| import RolePermissionsTree from "./permission-list"; | |||
| import { NestedPermissions } from "@/app/(app)/authorization/roles/[roleId]/tree"; | |||
There was a problem hiding this comment.
Use import type for type-only import of NestedPermissions
The import of NestedPermissions on line 29 is used only for type annotations. To optimize the build and avoid unnecessary module loading, use import type for type-only imports.
Apply this diff to fix the issue:
-import { NestedPermissions } from "@/app/(app)/authorization/roles/[roleId]/tree";
+import type { NestedPermissions } from "@/app/(app)/authorization/roles/[roleId]/tree";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { NestedPermissions } from "@/app/(app)/authorization/roles/[roleId]/tree"; | |
| import type { NestedPermissions } from "@/app/(app)/authorization/roles/[roleId]/tree"; |
🧰 Tools
🪛 Biome
[error] 28-29: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
| @@ -25,6 +25,9 @@ import ms from "ms"; | |||
| import { notFound } from "next/navigation"; | |||
| import { Chart } from "./chart"; | |||
| import { VerificationTable } from "./verification-table"; | |||
| import RolePermissionsTree from "./permission-list"; | |||
There was a problem hiding this comment.
Remove unused import RolePermissionsTree
The import RolePermissionsTree on line 28 is not used in this file. Removing it will clean up the code and prevent unnecessary module loading.
Apply this diff to fix the issue:
-import RolePermissionsTree from "./permission-list";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import RolePermissionsTree from "./permission-list"; |
🧰 Tools
🪛 Biome
[error] 28-29: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
| n = n[p].permissions; | ||
| } | ||
| } | ||
| let data = { |
There was a problem hiding this comment.
Use const instead of let for data
The variable data on line 185 is assigned once and not reassigned. Using const improves code clarity and prevents accidental mutations.
Apply this diff to fix the issue:
- let data = {
+ const data = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let data = { | |
| const data = { |
🧰 Tools
🪛 Biome
[error] 185-185: This let declares a variable that is only assigned once.
'data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/api/src/openapi.d.ts
Outdated
| * @description Set to true to automatically create the permissions they do not exist yet. Only works when specifying `name`. | ||
| * Autocreating roles requires your root key to have the `rbac.*.create_role` permission, otherwise the request will get rejected | ||
| */ | ||
| create?: boolean; |
There was a problem hiding this comment.
Incorrect reference to 'permissions' instead of 'roles'
In the roles property, the description for create incorrectly mentions 'permissions' instead of 'roles'. This could confuse developers using the API.
Apply this diff to fix the inconsistency:
- * @description Set to true to automatically create the permissions they do not exist yet. Only works when specifying `name`.
- * Autocreating roles requires your root key to have the `rbac.*.create_role` permission, otherwise the request will get rejected
+ * @description Set to true to automatically create the roles if they do not exist yet. Only works when specifying `name`.
+ * Autocreating roles requires your root key to have the `rbac.*.create_role` permission, otherwise the request will get rejected📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @description Set to true to automatically create the permissions they do not exist yet. Only works when specifying `name`. | |
| * Autocreating roles requires your root key to have the `rbac.*.create_role` permission, otherwise the request will get rejected | |
| */ | |
| create?: boolean; | |
| * @description Set to true to automatically create the roles if they do not exist yet. Only works when specifying `name`. | |
| * Autocreating roles requires your root key to have the `rbac.*.create_role` permission, otherwise the request will get rejected | |
| */ | |
| create?: boolean; |
|
There seems to be a lot going on in this PR that is unrelated to fixing the button, could you check? |
|
/assign |
|
Assigned to @nilaygit-10721! Please open a draft PR linking this issue within 48h |
|
Will run it |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx (1)
163-163: Consider renaming variablento improve readabilityThe variable
nserves as a pointer to the current node in the nested permissions tree. Renaming it to something more descriptive likecurrentNodeorcurrentLevelwould enhance code clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx (6 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx (1 hunks)
- apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (3 hunks)
- apps/dashboard/app/layout.tsx (1 hunks)
- apps/engineering/app/layout.tsx (1 hunks)
- apps/planetfall/app/layout.tsx (1 hunks)
- apps/workflows/app/layout.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx
- apps/dashboard/app/layout.tsx
- apps/engineering/app/layout.tsx
- apps/planetfall/app/layout.tsx
- apps/workflows/app/layout.tsx
🧰 Additional context used
🔇 Additional comments (4)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx (4)
1-11: Imports look good and are well-organized.The "use client" directive is correctly placed at the top, and all imports seem relevant to the component's functionality. The imports are well-organized, grouping local components, UI components, and React hooks.
13-36: Type definitions are well-structured and comprehensive.The type definitions for
NestedPermission,NestedPermissions,Role, andPermissionTreePropsare clear, well-structured, and exported for reusability. The use of theRecordtype inNestedPermissionsis appropriate for defining a dictionary-like structure.
38-113: Overall implementation is well-structured and functional.The PermissionTree component is well-implemented with proper state management using useState and useEffect hooks. The UI is nicely structured using various components, and the logic for expanding/collapsing roles is sound.
However, there are two minor issues to address:
- The
kprop in the RecursivePermission component (line 99) could be renamed for clarity.- The overflow behavior on the span element (line 84) might not work as expected.
I'll provide separate comments for these issues.
1-113: Overall, excellent implementation that aligns with PR objectivesThis PermissionTree component is well-implemented and aligns with the PR objectives of enhancing role and permission management. The code is well-structured, uses appropriate React hooks for state management, and provides a clear UI for displaying and interacting with roles and permissions.
Key strengths:
- Clear type definitions for better code maintainability.
- Efficient use of React hooks (useState, useEffect) for state management.
- Well-organized UI structure using various components.
- Implementation of expand/collapse functionality for all roles and individual roles.
The minor issues with prop naming and CSS classes, once addressed, will further improve the code quality. This implementation should significantly enhance the user experience in managing roles and permissions within the API key detail page.
| <span className="text-ellipsis overflow-hidden hover:overflow-visible"> | ||
| {role.name} | ||
| </span> |
There was a problem hiding this comment.
Ensure overflow behavior works correctly on inline elements
The overflow-hidden and hover:overflow-visible classes may not work as expected on a span element because it is an inline element. To ensure the overflow behavior functions correctly, consider changing the span to a div or adding inline-block to the class name.
Apply one of the following changes:
Option 1: Change span to div
-<span className="text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</span>
+<div className="text-ellipsis overflow-hidden hover:overflow-visible">{role.name}</div>Option 2: Add inline-block to the class name
-<span className="text-ellipsis overflow-hidden hover:overflow-visible">
+<span className="inline-block text-ellipsis overflow-hidden hover:overflow-visible">
{role.name}
</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span className="text-ellipsis overflow-hidden hover:overflow-visible"> | |
| {role.name} | |
| </span> | |
| <span className="inline-block text-ellipsis overflow-hidden hover:overflow-visible"> | |
| {role.name} | |
| </span> |
| <RecursivePermission | ||
| key={p.id} | ||
| k={k} | ||
| {...p} | ||
| roleId={role.id} | ||
| openAll={openAll} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename prop k to permissionKey for clarity
In the RecursivePermission component, consider renaming the prop k to permissionKey to improve readability and adhere to naming conventions.
Apply this diff:
<RecursivePermission
key={p.id}
- k={k}
+ permissionKey={k}
{...p}
roleId={role.id}
openAll={openAll}
/>Ensure that the RecursivePermission component is updated accordingly to accept the new permissionKey prop.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <RecursivePermission | |
| key={p.id} | |
| k={k} | |
| {...p} | |
| roleId={role.id} | |
| openAll={openAll} | |
| /> | |
| ))} | |
| <RecursivePermission | |
| key={p.id} | |
| permissionKey={k} | |
| {...p} | |
| roleId={role.id} | |
| openAll={openAll} | |
| /> | |
| ))} |
|
formatting done |
|
still blocked by #2238, unless you clean it up |
| .string() | ||
| .min(3) | ||
| .refine((v) => validCharactersRegex.test(v), { | ||
| message: "worksapce can only contain letters, numbers, dashes and underscores", |
| @@ -84,7 +92,7 @@ export const UpdateWorkspaceName: React.FC<Props> = ({ workspace }) => { | |||
| <Button | |||
| variant={updateName.isLoading ? "disabled" : "primary"} | |||
| type="submit" | |||
| disabled={updateName.isLoading} | |||
| disabled={updateName.isLoading || isDisabled} | |||
There was a problem hiding this comment.
put it all in here or all in a separate variable, but please don't do it half and half
apps/dashboard/app/layout.tsx
Outdated
| @@ -15,6 +15,8 @@ import { ThemeProvider } from "./theme-provider"; | |||
| const inter = Inter({ | |||
| subsets: ["latin"], | |||
| variable: "--font-inter", | |||
| display: "swap", | |||
There was a problem hiding this comment.
It's because of that previous pr
I will resolve all issues
Co-authored-by: Andreas Thomas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
oss.gg/7_create_a_template.md (1)
47-47: Fix formatting in the link text.The link text contains extra spaces, which can cause formatting issues. Please remove the extra spaces within the square brackets.
Apply this change:
-» 11-October-2024 by Prabin Subedi [Starter template for Unkey Route protecttion in Hono Webframework in Cloudflare workers ](https://github.com/prabincankod/hono-unkey-cflare) +» 11-October-2024 by Prabin Subedi [Starter template for Unkey Route protection in Hono Webframework in Cloudflare workers](https://github.com/prabincankod/hono-unkey-cflare)Also, note that "protection" is misspelled as "protecttion". Please correct this spelling error.
🧰 Tools
🪛 Markdownlint
47-47: null
Spaces inside link text(MD039, no-space-in-links)
apps/api/wrangler.toml (1)
7-8: Consider clarifying observability configuration across environmentsWhile the addition of observability configurations is a positive step, there are a few points to consider:
The global
[observability]section (lines 7-8) might affect all environments, while the production-specific section (lines 104-106) only affects the production environment. This could lead to confusion about which settings take precedence.Other environments (development, preview, canary) don't have explicit observability configurations. For consistency, consider adding similar configurations to these environments if observability should be enabled for them as well.
It's unclear if the intention is to enable observability for all environments or just production.
Recommendations:
- Clarify the intention behind having both global and production-specific configurations. If observability should be enabled for all environments, consider removing the production-specific section and relying on the global configuration.
- If different environments require different observability settings, add explicit configurations for each environment (development, preview, canary) to maintain consistency and clarity.
- Add comments in the configuration file to explain the purpose and impact of these new observability settings, especially if they differ between environments.
Would you like assistance in drafting these clarifications or additional configurations?
Also applies to: 104-106
apps/api/src/pkg/keys/service.ts (2)
253-253: Typo in comment: 'Createa' should be 'Create'There's a typo in the comment on line 253. It should read:
- * Createa a unique set of all permissions, whether they're attached directly or connected + * Create a unique set of all permissions, whether they're attached directly or connected
263-263: Typo in comment: 'pecedence' should be 'precedence'The comment on line 263 contains a typo. It should be corrected to:
- * Key limits take pecedence + * Key limits take precedence
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
- apps/api/package.json (2 hunks)
- apps/api/src/pkg/keys/service.ts (4 hunks)
- apps/api/wrangler.toml (2 hunks)
- apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx (3 hunks)
- apps/logdrain/package.json (1 hunks)
- apps/semantic-cache/package.json (1 hunks)
- oss.gg/7_create_a_template.md (1 hunks)
- packages/api/src/openapi.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx
🧰 Additional context used
🪛 Markdownlint
oss.gg/7_create_a_template.md
47-47: null
Spaces inside link text(MD039, no-space-in-links)
🔇 Additional comments (10)
apps/logdrain/package.json (1)
23-23: Verify the intentionality and impact of the wrangler update.The
wranglerdependency has been updated from version^3.62.0to^3.80.5. While keeping dependencies up-to-date is generally good practice, please confirm:
- Was this update intentional and related to the PR's main objective of disabling the save button when the workspace name is empty?
- Have you tested the application with this new version to ensure there are no breaking changes or conflicts with other dependencies?
To help verify the impact of this change, you can run the following command to check for any breaking changes or notable updates in the changelog:
This will help identify any significant changes that might affect your application.
apps/semantic-cache/package.json (1)
26-26: Approved: Wrangler dependency updateThe update of the
wranglerdependency from^3.62.0to^3.80.5is a good practice to keep the project up-to-date with the latest features and security patches.To ensure this update doesn't introduce any breaking changes, please run the following verification steps:
apps/api/package.json (2)
30-30: Confirm the addition of @unkey/clickhouse-zod dependency.The @unkey/clickhouse-zod package has been added to the dependencies. Please verify if this addition is intentional and related to the PR objectives. If it's part of the changes for disabling the save button when the workspace name is empty, could you provide more context on how it's being used?
#!/bin/bash # Description: Check for usage of @unkey/clickhouse-zod in the codebase # Test: Search for imports or uses of @unkey/clickhouse-zod rg --type typescript --type javascript '@unkey/clickhouse-zod'
21-21: Verify the intentional update of wrangler.The wrangler version has been updated from ^3.62.0 to ^3.80.5. While keeping dependencies up-to-date is generally good practice, please confirm if this update was intentional and if it has been tested with your current setup.
oss.gg/7_create_a_template.md (1)
49-49: LGTM!The new entry for the TypeScript + NestJS Web API template is correctly formatted and follows the required schema. It's a valuable addition to the list of Unkey templates.
apps/api/wrangler.toml (2)
7-8: New observability configuration addedThe addition of the
[observability]section withenabled = trueis a good step towards improving monitoring and debugging capabilities for the application.To ensure consistency across environments, let's check if similar configurations are present in other environment sections:
#!/bin/bash # Description: Check for observability configurations in other environments # Test: Search for observability configurations in other environments rg '\[env\..+\.observability\]' apps/api/wrangler.toml
104-106: Production-specific observability configuration addedThe addition of the
[env.production.observability]section withenabled = trueis appropriate for enabling observability features specifically in the production environment.Let's verify if this configuration aligns with the global observability setting and check for any potential conflicts:
✅ Verification successful
Observability Configuration Verified Successfully
Both global and production-specific observability settings are enabled and consistent. No conflicting configurations found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare global and production-specific observability settings # Test 1: Extract global observability setting echo "Global observability setting:" sed -n '/^\[observability\]/,/^$/p' apps/api/wrangler.toml # Test 2: Extract production-specific observability setting echo "Production-specific observability setting:" sed -n '/^\[env\.production\.observability\]/,/^$/p' apps/api/wrangler.toml # Test 3: Check for any conflicting settings in the production environment echo "Checking for conflicts in production environment:" sed -n '/^\[env\.production\]/,/^\[/p' apps/api/wrangler.toml | grep -i 'observability'Length of output: 87
Script:
#!/bin/bash # Description: Verify global observability settings and check for conflicts in production environment # Extract global observability settings echo "Global observability setting:" sed -n '/^\[observability\]/,/^$/p' apps/api/wrangler.toml # Extract production-specific observability settings echo "Production-specific observability setting:" sed -n '/^\[env\.production\.observability\]/,/^$/p' apps/api/wrangler.toml # Check for conflicting observability settings in production environment echo "Checking for conflicts in production environment:" sed -n '/^\[env\.production\]/,/^\[/p' apps/api/wrangler.toml | grep -i 'observability'Length of output: 577
apps/api/src/pkg/keys/service.ts (3)
185-295: NewgetDatamethod improves modularity and readabilityThe introduction of the
getDatamethod effectively encapsulates the data retrieval and processing logic. This refactoring enhances code modularity, making it easier to manage and maintain.
318-320: Refactored_verifyKeymethod enhances error handlingThe updated
_verifyKeymethod now accepts an optionaloptsparameter, allowing for more granular control over cache usage. The implementation correctly handles theskipCacheoption, improving the robustness of error handling and retry logic.Also applies to: 329-359
145-151: 🛠️ Refactor suggestionConsider clearing the cache before retrying without cache
In the
verifyKeymethod, when an error occurs, you retry_verifyKeywith{ skipCache: true }but do not remove the stale cache entry. This might lead to future calls using outdated data. It's advisable to clear the cache entry before retrying to ensure consistent data retrieval.Apply this diff to remove the cache entry before retrying:
const res = await this._verifyKey(c, req).catch(async (err) => { this.logger.error("verify error, retrying without cache", { error: err.message, }); + await this.cache.keyByHash.remove(await this.hash(req.key)); return await this._verifyKey(c, req, { skipCache: true }); });Likely invalid or redundant comment.
|
Awarding RajuGangitla: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/RajuGangitla |
What does this PR do?
This pr will a add validation when workspace name is empty
Fixes #2260
screen-capture (21).webm
Summary by CodeRabbit
New Features
PermissionTreecomponent for improved display of roles and permissions.createApi,listKeys, anddeleteApi.Bug Fixes
Documentation
Chores