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
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { Button, buttonVariants } from "@/components/ui/button";
import { Card, CardContent, CardHeader } from "@/components/ui/card";
import { Separator } from "@/components/ui/separator";
import { getTenantId } from "@/lib/auth";
import { and, db, eq, isNull, schema } from "@/lib/db";
import { and, db, eq, isNull, Role, schema } from "@/lib/db";
import { formatNumber } from "@/lib/fmt";
import {
getLastUsed,
Expand All @@ -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)

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)

import PermissionTree from "./permission-list";

export default async function APIKeyDetailPage(props: {
params: {
Expand Down Expand Up @@ -71,6 +74,8 @@ export default async function APIKeyDetailPage(props: {
},
},
});


if (!key || key.workspace.tenantId !== tenantId) {
return notFound();
}
Expand Down Expand Up @@ -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


const nested: NestedPermissions = {};
for (const permission of key.workspace.permissions) {
let n = nested;
const parts = permission.name.split(".");
for (let i = 0; i < parts.length; i++) {
const p = parts[i];
if (!(p in n)) {
n[p] = {
id: permission.id,
name: permission.name,
description: permission.description,
checked: role.permissions.some((p) => p.permissionId === permission.id),
part: p,
permissions: {},
path: parts.slice(0, i).join("."),
};
}
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)

id: role.id,
name: role.name,
description: role.description,
keyId: key.id,
active: key.roles.some((keyRole) => keyRole.roleId === role.id),
nestedPermissions: nested
}
return data
})



return (
<div className="flex flex-col">
<div className="flex items-center justify-between w-full">
Expand Down Expand Up @@ -308,7 +348,8 @@ export default async function APIKeyDetailPage(props: {
</div>
</div>

<Chart
<PermissionTree roles={roleTee} />
{/* <Chart
apiId={props.params.apiId}
key={JSON.stringify(key)}
data={key}
Expand All @@ -320,7 +361,7 @@ export default async function APIKeyDetailPage(props: {
...p,
active: transientPermissionIds.has(p.id),
}))}
/>
/> */}
</div>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
"use client"
import React, { useState, useEffect } from 'react';
import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card";
import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "@/components/ui/collapsible";
import { Label } from "@/components/ui/label";
import { Switch } from "@/components/ui/switch";
import { RecursivePermission } from '@/app/(app)/authorization/roles/[roleId]/tree';
import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip";
import { CopyButton } from '@/components/dashboard/copy-button';
import { RoleToggle } from './role-toggle';
import { ChevronRight } from 'lucide-react';

export type NestedPermission = {
id: string;
checked: boolean;
description: string | null;
name: string;
part: string;
path: string;
permissions: NestedPermissions;
};

export type NestedPermissions = Record<string, NestedPermission>;

export type Role = {
id: string;
name: string;
keyId: string
active: boolean
description: string | null;
nestedPermissions: NestedPermissions;
};

type PermissionTreeProps = {
roles: Role[];
};


export default function PermissionTree({ roles }: PermissionTreeProps) {
const [openAll, setOpenAll] = useState(false);
const [openRoles, setOpenRoles] = useState<string[]>([]);

useEffect(() => {
setOpenRoles(openAll ? roles.map(role => role.id) : []);
}, [openAll, roles]);

return (
<Card>
<CardHeader className="flex-row items-start justify-between">
<div className="flex flex-col space-y-1.5">
<CardTitle>Permissions</CardTitle>
</div>
<div className="flex items-center gap-2">
<Label>{openAll ? "Collapse" : "Expand"} All Roles</Label>
<Switch checked={openAll} onCheckedChange={setOpenAll} />
</div>
</CardHeader>

<CardContent className="flex flex-col gap-4">
{roles.map(role => {
const isOpen = openRoles.includes(role.id);
return (
<Collapsible
key={role.id}
open={isOpen}
onOpenChange={(open) => {
setOpenRoles(prev =>
open
? prev.includes(role.id) ? prev : [...prev, role.id]
: prev.filter(id => id !== role.id)
);
}}
>
<CollapsibleTrigger className="flex items-center gap-1 transition-all [&[data-state=open]>svg]:rotate-90 ">
<Tooltip delayDuration={50}>
<TooltipTrigger className="flex items-center gap-2">
<ChevronRight className="w-4 h-4 transition-transform duration-200" />
<RoleToggle keyId={role.keyId} roleId={role.id} checked={role.active} />
<span className="text-sm">{role.name}</span>
</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>

<div>
<CopyButton value={role.name} />
</div>
</div>
</TooltipContent>
</Tooltip>

</CollapsibleTrigger>
<CollapsibleContent className="pt-2 pb-4">
<div className="flex flex-col gap-1 ml-4">
{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.

{...p}
roleId={role.id}
openAll={openAll}
/>
))}
</div>
</CollapsibleContent>
</Collapsible>
);
})}
</CardContent>
</Card>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export const Tree: React.FC<Props> = ({ nestedPermissions, role }) => {
);
};

const RecursivePermission: React.FC<
export const RecursivePermission: React.FC<
NestedPermission & { k: string; roleId: string; openAll: boolean }
> = ({ k, openAll, id, name, permissions, roleId, checked, description }) => {
const [open, setOpen] = useState(openAll);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,15 @@ import { useForm } from "react-hook-form";
import { z } from "zod";

export const dynamic = "force-dynamic";

const validCharactersRegex = /^[a-zA-Z0-9-_]+$/;

const formSchema = z.object({
workspaceId: z.string(),
name: z.string(),
name: z.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

}),
});

type Props = {
Expand Down Expand Up @@ -53,7 +59,7 @@ export const UpdateWorkspaceName: React.FC<Props> = ({ workspace }) => {
async function onSubmit(values: z.infer<typeof formSchema>) {
await updateName.mutateAsync(values);
}

const isDisabled = form.formState.isLoading || !form.formState.isValid;
return (
<Form {...form}>
<form onSubmit={form.handleSubmit(onSubmit)}>
Expand Down Expand Up @@ -84,7 +90,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

>
{updateName.isLoading ? <Loading /> : "Save"}
</Button>
Expand Down
2 changes: 2 additions & 0 deletions apps/dashboard/app/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { ThemeProvider } from "./theme-provider";
const inter = Inter({
subsets: ["latin"],
variable: "--font-inter",
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

});

const pangea = localFont({
Expand Down
2 changes: 2 additions & 0 deletions apps/engineering/app/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import "./global.css";

const inter = Inter({
subsets: ["latin"],
display: 'swap',
adjustFontFallback: false
});

export default function Layout({ children }: { children: ReactNode }) {
Expand Down
6 changes: 5 additions & 1 deletion apps/planetfall/app/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import type { Metadata } from "next";
import { Inter } from "next/font/google";
import "./globals.css";

const inter = Inter({ subsets: ["latin"] });
const inter = Inter({
subsets: ["latin"],
display: 'swap',
adjustFontFallback: false
});

export const metadata: Metadata = {
title: "Create Next App",
Expand Down
6 changes: 5 additions & 1 deletion apps/workflows/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import type { Metadata } from "next";
import { Inter } from "next/font/google";

const inter = Inter({ subsets: ["latin"] });
const inter = Inter({
subsets: ["latin"],
display: 'swap',
adjustFontFallback: false
});

export const metadata: Metadata = {
title: "Create Next App",
Expand Down
Loading