Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: disable save button when workspace name is empty #2294

Merged
merged 18 commits into from
Oct 20, 2024

Conversation

RajuGangitla
Copy link
Contributor

@RajuGangitla RajuGangitla commented Oct 10, 2024

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

    • Enhanced role and permission management on the API key detail page.
    • Introduced PermissionTree component for improved display of roles and permissions.
    • Updated workspace name validation with new regex and minimum length requirements.
    • Added new API endpoints: createApi, listKeys, and deleteApi.
  • Bug Fixes

    • Improved font rendering behavior across various applications.
  • Documentation

    • Updated TypeScript definitions for clarity and added new API endpoints.
  • Chores

    • Enhanced internal logic for managing permissions while maintaining existing verification data handling.

Copy link

changeset-bot bot commented Oct 10, 2024

⚠️ No Changeset found

Latest commit: 537064e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Oct 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
engineering ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 20, 2024 7:07am

Copy link
Contributor

coderabbitai bot commented Oct 10, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes in this pull request enhance role and permission management within the API key detail page, introduce a new PermissionTree component for displaying permissions, and modify the UpdateWorkspaceName component to enforce validation on workspace names. Additionally, font configurations across various layout files have been updated for improved rendering. TypeScript definitions have been refined, and new API endpoints have been added to the OpenAPI specification.

Changes

File Change Summary
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/page.tsx Enhanced role and permission management; replaced Chart component with PermissionTree; expanded logic for roles and permissions.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/permission-list.tsx Introduced PermissionTree component for displaying hierarchical permissions; added TypeScript types for clarity.
apps/dashboard/app/(app)/settings/general/update-workspace-name.tsx Updated workspace name validation logic; introduced regex for character validation and updated form schema.
apps/dashboard/app/layout.tsx Updated Inter font configuration with new properties for rendering.
apps/engineering/app/layout.tsx Updated Layout component with new Inter font properties.
apps/planetfall/app/layout.tsx Expanded Inter font instantiation with new properties.
apps/workflows/app/layout.tsx Modified Inter font instantiation to include new rendering properties.
packages/api/src/openapi.d.ts Updated TypeScript definitions; added new API endpoints and refined existing types for clarity and consistency.

Assessment against linked issues

Objective Addressed Explanation
Disable Save Button if workspace name is empty (#2260)

Possibly related PRs

Suggested labels

Good first issue, hacktoberfest

Suggested reviewers

  • mcstepp
  • chronark
  • perkinsjr
  • MichaelUnkey

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

vercel bot commented Oct 10, 2024

@RajuGangitla is attempting to deploy a commit to the Unkey Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

github-actions bot commented Oct 10, 2024

Thank you for following the naming conventions for pull request titles! 🙏

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 correction

The updates to the formSchema for the name field are good:

  • The minimum length requirement of 3 characters is appropriate.
  • Using refine with validCharactersRegex ensures 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 exported

The change to export the RecursivePermission component 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 useEffect

Including roles in the dependency array of the useEffect hook may cause unnecessary re-renders if the roles array reference changes, even when its contents are the same. If roles does not change frequently or is already memoized upstream, consider removing it from the dependency array to prevent potential performance issues.


55-55: Add aria-label to Switch component for accessibility

Consider adding an aria-label to the Switch component 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 optional limit and duration in ratelimits

The limit and duration properties 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 in async property description

There's a typo in the description of the async property: "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 in meta description

The 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

📥 Commits

Files that changed from the base of the PR and between b68e117 and fa6b514.

📒 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:

  1. display: 'swap' will show a fallback font until Inter is loaded, improving perceived performance.
  2. adjustFontFallback: false disables 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.tsx are 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" --oneline

Could 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 Inter font configuration introduce two new properties:

  1. display: 'swap': This will show text immediately using a fallback font while loading the custom font, potentially causing a noticeable font change (FOUT).
  2. 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:

  1. display: 'swap' can improve perceived loading performance by showing content quickly.
  2. adjustFontFallback: false gives 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 adjustFontFallback might 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 validation

The validCharactersRegex constant 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 logic

The isDisabled constant 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 state

The update to the submit button's disabled property correctly implements the PR objective. By using both updateName.isLoading and the isDisabled constant, the button will be disabled when:

  1. The form is being submitted
  2. 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 needed

The changes in this file successfully implement the PR objectives:

  1. The save button is now disabled when the workspace name is empty.
  2. 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] Importing Role from @/lib/db

The addition of Role in the import statement is appropriate for the enhanced role management functionality.

packages/api/src/openapi.d.ts (5)

9-10: Type definitions for XOR and OneOf are correctly implemented

The updated type definitions enhance type safety and are well-structured for representing exclusive and one-of type constraints.


524-524: Update to code property in V1KeysVerifyKeyResponse is appropriate

Defining the code property as a union of string literals clearly enumerates all possible status codes, improving type correctness and readability.


550-554: PermissionQuery type definition is clear and recursive

The use of OneOf and the recursive structure allows for complex permission queries combining strings with logical and/or operators.


1341-1349: refill property definition is precise and clear

The refill option is well-defined, allowing for automatic refill settings with clear interval choices.


1558-1579: Addition of detailed usage metrics in verifications

Including properties like success, rateLimited, and usageExceeded provides valuable insights for monitoring and analytics.

Comment on lines 18 to 19
display: 'swap',
adjustFontFallback: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
<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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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 = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)

Comment on lines 1377 to 1380
* @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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
* @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;

@chronark
Copy link
Collaborator

There seems to be a lot going on in this PR that is unrelated to fixing the button, could you check?

@RajuGangitla
Copy link
Contributor Author

@chronark it is because of my other pr which was not merged #2238
and im getting that same code here also
can you merge that pr first

@nilaygit-10721
Copy link

/assign

Copy link

oss-gg bot commented Oct 10, 2024

Assigned to @nilaygit-10721! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@RajuGangitla
Copy link
Contributor Author

Will run it

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 variable n to improve readability

The variable n serves as a pointer to the current node in the nested permissions tree. Renaming it to something more descriptive like currentNode or currentLevel would enhance code clarity and maintainability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between fa6b514 and 8baaeca.

📒 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, and PermissionTreeProps are clear, well-structured, and exported for reusability. The use of the Record type in NestedPermissions is 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:

  1. The k prop in the RecursivePermission component (line 99) could be renamed for clarity.
  2. 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 objectives

This 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:

  1. Clear type definitions for better code maintainability.
  2. Efficient use of React hooks (useState, useEffect) for state management.
  3. Well-organized UI structure using various components.
  4. 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.

Comment on lines 84 to 86
<span className="text-ellipsis overflow-hidden hover:overflow-visible">
{role.name}
</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
<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>

Comment on lines 97 to 104
<RecursivePermission
key={p.id}
k={k}
{...p}
roleId={role.id}
openAll={openAll}
/>
))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
<RecursivePermission
key={p.id}
k={k}
{...p}
roleId={role.id}
openAll={openAll}
/>
))}
<RecursivePermission
key={p.id}
permissionKey={k}
{...p}
roleId={role.id}
openAll={openAll}
/>
))}

@RajuGangitla
Copy link
Contributor Author

RajuGangitla commented Oct 11, 2024

formatting done

@chronark
Copy link
Collaborator

still blocked by #2238, unless you clean it up

@chronark chronark self-assigned this Oct 13, 2024
@RajuGangitla
Copy link
Contributor Author

@chronark i resolved #2238 after merging that issue i will resolve this one

.string()
.min(3)
.refine((v) => validCharactersRegex.test(v), {
message: "worksapce can only contain letters, numbers, dashes and underscores",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@@ -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}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put it all in here or all in a separate variable, but please don't do it half and half

@@ -15,6 +15,8 @@ import { ThemeProvider } from "./theme-provider";
const inter = Inter({
subsets: ["latin"],
variable: "--font-inter",
display: "swap",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because of that previous pr

I will resolve all issues

apps/engineering/app/layout.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 environments

While the addition of observability configurations is a positive step, there are a few points to consider:

  1. 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.

  2. 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.

  3. It's unclear if the intention is to enable observability for all environments or just production.

Recommendations:

  1. 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.
  2. If different environments require different observability settings, add explicit configurations for each environment (development, preview, canary) to maintain consistency and clarity.
  3. 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

📥 Commits

Files that changed from the base of the PR and between 8baaeca and c3dab41.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is 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 wrangler dependency has been updated from version ^3.62.0 to ^3.80.5. While keeping dependencies up-to-date is generally good practice, please confirm:

  1. Was this update intentional and related to the PR's main objective of disabling the save button when the workspace name is empty?
  2. 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 update

The update of the wrangler dependency from ^3.62.0 to ^3.80.5 is 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 added

The addition of the [observability] section with enabled = true is 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 added

The addition of the [env.production.observability] section with enabled = true is 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: New getData method improves modularity and readability

The introduction of the getData method effectively encapsulates the data retrieval and processing logic. This refactoring enhances code modularity, making it easier to manage and maintain.


318-320: Refactored _verifyKey method enhances error handling

The updated _verifyKey method now accepts an optional opts parameter, allowing for more granular control over cache usage. The implementation correctly handles the skipCache option, improving the robustness of error handling and retry logic.

Also applies to: 329-359


145-151: 🛠️ Refactor suggestion

Consider clearing the cache before retrying without cache

In the verifyKey method, when an error occurs, you retry _verifyKey with { 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.

oss.gg/7_create_a_template.md Outdated Show resolved Hide resolved
packages/api/src/openapi.d.ts Outdated Show resolved Hide resolved
apps/api/src/pkg/keys/service.ts Outdated Show resolved Hide resolved
@chronark chronark merged commit 040ee84 into unkeyed:main Oct 20, 2024
18 of 23 checks passed
Copy link

oss-gg bot commented Oct 20, 2024

Awarding RajuGangitla: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/RajuGangitla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Save Button if workspace name is empty
5 participants