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.

Sign up for GitHub

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

init sidebar refactor #218

Merged
merged 3 commits into from
Nov 18, 2024
Merged

init sidebar refactor #218

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f2957d
init sidebar refactor
jsbroks Nov 18, 2024
7311d27
Merge branch 'main' into refactor-sidebar
jsbroks Nov 18, 2024
db55f03
move layout around
jsbroks Nov 18, 2024
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
134 changes: 134 additions & 0 deletions apps/webservice/src/app/[workspaceSlug]/AppSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import type { Workspace } from "@ctrlplane/db/schema";
import React from "react";
import {
IconCategory,
IconObjectScan,
IconPlant,
IconRocket,
IconRun,
IconShip,
IconVariable,
} from "@tabler/icons-react";

import { SidebarContent, SidebarHeader } from "@ctrlplane/ui/sidebar";

import { api } from "~/trpc/server";
import { AppSidebarHeader } from "./AppSidebarHeader";
import { AppSidebarPopover } from "./AppSidebarPopover";
import { SidebarWithPopover } from "./AppSidebarPopoverContext";
import { SidebarNavMain } from "./SidebarNavMain";

const navMain = (prefix: string) => [
{
title: "Systems",
url: `${prefix}/systems`,
icon: IconCategory,
isActive: true,
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 hardcoded isActive states

The isActive property is hardcoded to true for multiple navigation items. This should be determined dynamically based on the current route.

Consider implementing a utility function to determine the active state:

const isRouteActive = (currentPath: string, itemPath: string) => 
  currentPath.startsWith(itemPath);

Also applies to: 31-31, 40-40, 89-89

items: [
{
title: "Dependencies",
url: `${prefix}/dependencies`,
isActive: true,
},
],
},
{
title: "Resources",
url: `${prefix}/resources`,
popoverId: "resources",
icon: IconObjectScan,
isActive: true,
items: [
{
title: "List",
url: `${prefix}/targets`,
},
{
title: "Providers",
url: `${prefix}/target-providers`,
},
{
title: "Groups",
url: `${prefix}/target-metadata-groups`,
},
{
title: "Views",
url: `${prefix}/target-views`,
},
],
},
{
title: "Jobs",
url: "#",
icon: IconRocket,
items: [
{
title: "Agents",
url: "#",
},
{
title: "Runs",
url: "#",
},
],
},
];
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

Based on the findings, I can now provide a final response about the placeholder URLs in the navigation.

Update navigation URLs to match implemented routes

The placeholder URLs in AppSidebar.tsx for the Jobs section should be updated to match the implemented routes:

{
    title: "Jobs",
    url: `${prefix}/job-agents`, // Update from "#"
    icon: IconRocket,
    items: [
      {
        title: "Agents",
        url: `${prefix}/job-agents`, // Update from "#"
      },
      {
        title: "Runs",
        url: `${prefix}/jobs`, // Update from "#"
      },
    ],
  }

The routes are fully implemented with corresponding pages:

  • /[workspaceSlug]/job-agents - Job agents listing and management
  • /[workspaceSlug]/jobs - Job runs listing and monitoring
🔗 Analysis chain

Review and implement placeholder navigation URLs

Several navigation items are using "#" as placeholder URLs. These should be implemented with proper routing paths.

Would you like help implementing the proper routing paths based on the application's routing structure?

Let me analyze the findings from the router definitions and navigation paths. The search results show that the application has well-defined routers in the backend (packages/api/src/router/) and uses Next.js routing in the frontend. Let me verify the specific routes for the "Jobs" section.

Let me check the specific job-related pages and routes to verify the implementation.

Let me try a different approach to check the job-related implementations.

Let me check the actual page implementations to verify the routes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for route definitions to ensure consistency
rg -g '*.ts*' 'createNextRouter|Route|router\.' -A 3

Length of output: 167700

Script:

#!/bin/bash
# Search for job-related routes and pages
rg -g '*.tsx' -g '*.ts' 'jobs|agents|runs' apps/webservice/src/app/\[workspaceSlug\]/ -l

Length of output: 3748

Script:

#!/bin/bash
# Check the job-related pages and routes
rg -g '*.tsx' -g '*.ts' 'page.tsx|layout.tsx' apps/webservice/src/app/\[workspaceSlug\]/\(job\)/ -A 5

Length of output: 101

Script:

#!/bin/bash
# Check contents of job-related components
cat apps/webservice/src/app/\[workspaceSlug\]/\(job\)/jobs/JobsGettingStarted.tsx apps/webservice/src/app/\[workspaceSlug\]/\(job\)/job-agents/JobAgentsGettingStarted.tsx 2>/dev/null

# Also check for any route definitions in the job folder
find apps/webservice/src/app/\[workspaceSlug\]/\(job\)/ -type f -name "page.tsx" -o -name "layout.tsx" 2>/dev/null

Length of output: 3323

Script:

#!/bin/bash
# Check the contents of the job-related pages
cat apps/webservice/src/app/\[workspaceSlug\]/\(job\)/jobs/page.tsx apps/webservice/src/app/\[workspaceSlug\]/\(job\)/job-agents/page.tsx apps/webservice/src/app/\[workspaceSlug\]/\(job\)/layout.tsx 2>/dev/null

Length of output: 5794


export const AppSidebar: React.FC<{ workspace: Workspace }> = async ({
workspace,
}) => {
const [workspaces, viewer, systems] = await Promise.all([
api.workspace.list(),
api.user.viewer(),
api.system.list({ workspaceId: workspace.id }),
]);
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

Add error handling for API calls

The Promise.all call lacks error handling, which could lead to unhandled rejections. Consider implementing proper error boundaries or try-catch blocks.

-  const [workspaces, viewer, systems] = await Promise.all([
-    api.workspace.list(),
-    api.user.viewer(),
-    api.system.list({ workspaceId: workspace.id }),
-  ]);
+  try {
+    const [workspaces, viewer, systems] = await Promise.all([
+      api.workspace.list(),
+      api.user.viewer(),
+      api.system.list({ workspaceId: workspace.id }),
+    ]);
+  } catch (error) {
+    // Consider implementing error boundary or error state
+    console.error('Failed to fetch sidebar data:', error);
+    throw error;
+  }

Committable suggestion skipped: line range outside the PR's diff.


const navSystem = systems.items.map((s) => ({
title: s.name,
url: "#",
isActive: true,
items: [
{
title: "Environments",
icon: IconPlant,
url: "#",
},
{
title: "Deployements",
icon: IconShip,
url: "#",
},
{
title: "Runbooks",
icon: IconRun,
url: "#",
},
{
title: "Variable Sets",
icon: IconVariable,
url: "#",
},
],
}));

return (
<SidebarWithPopover>
<AppSidebarPopover />
<SidebarHeader>
<AppSidebarHeader
workspace={workspace}
workspaces={workspaces}
viewer={viewer}
systems={systems.items}
/>
</SidebarHeader>
<SidebarContent>
<SidebarNavMain
title="Workspace"
items={navMain(`/${workspace.slug}`)}
/>
<SidebarNavMain title="Systems" items={navSystem} />
</SidebarContent>
</SidebarWithPopover>
);
};
47 changes: 39 additions & 8 deletions ...app/[workspaceSlug]/SidebarCreateMenu.tsx → .../[workspaceSlug]/AppSidebarCreateMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
"use client";

import type { Workspace } from "@ctrlplane/db/schema";
import { useState } from "react";
import { useParams } from "next/navigation";
import { IconPlus } from "@tabler/icons-react";

import { Button } from "@ctrlplane/ui/button";
Expand All @@ -12,17 +15,35 @@ import {
DropdownMenuTrigger,
} from "@ctrlplane/ui/dropdown-menu";

import { api } from "~/trpc/react";
import { CreateDeploymentDialog } from "./_components/CreateDeployment";
import { CreateReleaseDialog } from "./_components/CreateRelease";
import { CreateSystemDialog } from "./_components/CreateSystem";
import { CreateTargetDialog } from "./_components/CreateTarget";
import { CreateSessionDialog } from "./_components/terminal/CreateDialogSession";

export const SidebarCreateMenu: React.FC<{
export const AppSidebarCreateMenu: React.FC<{
workspace: Workspace;
deploymentId?: string;
systemId?: string;
}> = (props) => {
}> = ({ workspace }) => {
const { deploymentSlug, workspaceSlug, systemSlug } = useParams<{
workspaceSlug: string;
systemSlug?: string;
deploymentSlug?: string;
}>();

const system = api.system.bySlug.useQuery(
{ workspaceSlug, systemSlug: systemSlug ?? "" },
{ enabled: systemSlug != null },
);
const deployment = api.deployment.bySlug.useQuery(
{
workspaceSlug,
systemSlug: systemSlug ?? "",
deploymentSlug: deploymentSlug ?? "",
},
{ enabled: deploymentSlug != null && systemSlug != null },
);

const [open, setOpen] = useState(false);
return (
<DropdownMenu open={open} onOpenChange={setOpen}>
Expand All @@ -42,19 +63,26 @@ export const SidebarCreateMenu: React.FC<{
>
<DropdownMenuGroup>
<CreateSystemDialog
workspace={props.workspace}
workspace={workspace}
onSuccess={() => setOpen(false)}
>
<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
New System
</DropdownMenuItem>
</CreateSystemDialog>
<CreateDeploymentDialog {...props} onSuccess={() => setOpen(false)}>
<CreateDeploymentDialog
systemId={system.data?.id}
onSuccess={() => setOpen(false)}
>
<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
New Deployment
</DropdownMenuItem>
</CreateDeploymentDialog>
<CreateReleaseDialog {...props} onClose={() => setOpen(false)}>
<CreateReleaseDialog
systemId={system.data?.id}
deploymentId={deployment.data?.id}
onClose={() => setOpen(false)}
>
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

Prevent potential race conditions with query data.

The dialogs might be opened before query data is available. Consider disabling the menu items when the required data is not yet loaded.

 <CreateDeploymentDialog
   systemId={system.data?.id}
   onSuccess={() => setOpen(false)}
 >
-  <DropdownMenuItem onSelect={(e) => e.preventDefault()}>
+  <DropdownMenuItem 
+    onSelect={(e) => e.preventDefault()}
+    disabled={!system.data?.id}
+  >
     New Deployment
   </DropdownMenuItem>
 </CreateDeploymentDialog>
 <CreateReleaseDialog
   systemId={system.data?.id}
   deploymentId={deployment.data?.id}
   onClose={() => setOpen(false)}
 >
-  <DropdownMenuItem onSelect={(e) => e.preventDefault()}>
+  <DropdownMenuItem 
+    onSelect={(e) => e.preventDefault()}
+    disabled={!system.data?.id || !deployment.data?.id}
+  >
     New Release
   </DropdownMenuItem>
 </CreateReleaseDialog>
📝 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
<CreateDeploymentDialog
systemId={system.data?.id}
onSuccess={() => setOpen(false)}
>
<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
New Deployment
</DropdownMenuItem>
</CreateDeploymentDialog>
<CreateReleaseDialog {...props} onClose={() => setOpen(false)}>
<CreateReleaseDialog
systemId={system.data?.id}
deploymentId={deployment.data?.id}
onClose={() => setOpen(false)}
>
<CreateDeploymentDialog
systemId={system.data?.id}
onSuccess={() => setOpen(false)}
>
<DropdownMenuItem
onSelect={(e) => e.preventDefault()}
disabled={!system.data?.id}
>
New Deployment
</DropdownMenuItem>
</CreateDeploymentDialog>
<CreateReleaseDialog
systemId={system.data?.id}
deploymentId={deployment.data?.id}
onClose={() => setOpen(false)}
>
<DropdownMenuItem
onSelect={(e) => e.preventDefault()}
disabled={!system.data?.id || !deployment.data?.id}
>
New Release
</DropdownMenuItem>
</CreateReleaseDialog>

<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
New Release
</DropdownMenuItem>
Expand All @@ -75,7 +103,10 @@ export const SidebarCreateMenu: React.FC<{
<DropdownMenuSeparator />

<DropdownMenuGroup>
<CreateTargetDialog {...props} onSuccess={() => setOpen(false)}>
<CreateTargetDialog
workspace={workspace}
onSuccess={() => setOpen(false)}
>
<DropdownMenuItem onSelect={(e) => e.preventDefault()}>
Bootstrap Target
</DropdownMenuItem>
Expand Down
50 changes: 40 additions & 10 deletions ...rkspaceSlug]/SidebarWorkspaceDropdown.tsx → .../app/[workspaceSlug]/AppSidebarHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
"use client";

import type { Workspace } from "@ctrlplane/db/schema";
import type { System, Workspace } from "@ctrlplane/db/schema";
import Link from "next/link";
import { IconCheck, IconChevronDown } from "@tabler/icons-react";
import { signOut, useSession } from "next-auth/react";
import { IconCheck, IconChevronDown, IconSearch } from "@tabler/icons-react";
import { signOut } from "next-auth/react";

import { Button } from "@ctrlplane/ui/button";
import {
Expand All @@ -19,14 +19,17 @@ import {
DropdownMenuSubTrigger,
DropdownMenuTrigger,
} from "@ctrlplane/ui/dropdown-menu";
import { SidebarMenu, SidebarMenuItem } from "@ctrlplane/ui/sidebar";

import { api } from "~/trpc/react";
import { SearchDialog } from "./_components/SearchDialog";
import { AppSidebarCreateMenu } from "./AppSidebarCreateMenu";

export const SidebarWorkspaceDropdown: React.FC<{ workspace: Workspace }> = ({
workspace,
}) => {
const { data } = useSession();
const workspaces = api.workspace.list.useQuery();
const WorkspaceDropdown: React.FC<{
workspace: Workspace;
workspaces: Workspace[];
viewer: { email: string };
}> = ({ workspace, workspaces, viewer }) => {
const update = api.profile.update.useMutation();
return (
<DropdownMenu>
Expand All @@ -53,9 +56,9 @@ export const SidebarWorkspaceDropdown: React.FC<{ workspace: Workspace }> = ({
<DropdownMenuPortal>
<DropdownMenuSubContent className="w-[200px] bg-neutral-900">
<DropdownMenuLabel className="font-normal text-muted-foreground">
{data?.user.email}
{viewer.email}
</DropdownMenuLabel>
{workspaces.data?.map((ws) => (
{workspaces.map((ws) => (
<Link
key={ws.id}
href={`/${ws.slug}`}
Expand Down Expand Up @@ -85,3 +88,30 @@ export const SidebarWorkspaceDropdown: React.FC<{ workspace: Workspace }> = ({
</DropdownMenu>
);
};

export const AppSidebarHeader: React.FC<{
systems: System[];
workspace: Workspace;
workspaces: Workspace[];
viewer: { email: string };
}> = ({ workspace, workspaces, viewer }) => {
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

Remove unused systems prop

The systems prop is declared but never used in the component.

 export const AppSidebarHeader: React.FC<{
-  systems: System[];
   workspace: Workspace;
   workspaces: Workspace[];
   viewer: { email: string };
 }> = ({ workspace, workspaces, viewer }) => {
📝 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
export const AppSidebarHeader: React.FC<{
systems: System[];
workspace: Workspace;
workspaces: Workspace[];
viewer: { email: string };
}> = ({ workspace, workspaces, viewer }) => {
export const AppSidebarHeader: React.FC<{
workspace: Workspace;
workspaces: Workspace[];
viewer: { email: string };
}> = ({ workspace, workspaces, viewer }) => {

return (
<SidebarMenu>
<SidebarMenuItem className="flex items-center gap-2">
<div className="flex-grow overflow-x-auto">
<WorkspaceDropdown
workspace={workspace}
workspaces={workspaces}
viewer={viewer}
/>
</div>
<SearchDialog>
<Button variant="ghost" size="icon" className="h-6 w-6 flex-shrink-0">
<IconSearch className="h-4 w-4" />
</Button>
</SearchDialog>
<AppSidebarCreateMenu workspace={workspace} />
</SidebarMenuItem>
</SidebarMenu>
);
};
23 changes: 23 additions & 0 deletions apps/webservice/src/app/[workspaceSlug]/AppSidebarPopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
"use client";

import React from "react";

import { Popover, PopoverAnchor, PopoverContent } from "@ctrlplane/ui/popover";

import { useSidebarPopover } from "./AppSidebarPopoverContext";

export const AppSidebarPopover: React.FC = () => {
const { activeSidebarItem } = useSidebarPopover();
return (
<Popover open={activeSidebarItem != null}>
<PopoverAnchor className="w-full" />
<PopoverContent
side="right"
sideOffset={1}
className="h-[100vh] w-[300px] border-y-0 border-l-0 bg-black"
>
{activeSidebarItem}
</PopoverContent>
</Popover>
);
};
Loading
Loading