Skip to content

Conversation

@adityachoudhari26
Copy link
Contributor

@adityachoudhari26 adityachoudhari26 commented Nov 17, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced DepEdge component for custom edges in flow diagrams.
    • Added ResourceVisualizationDiagram and ResourceVisualizationDiagramProvider for visualizing resource relationships.
    • Implemented various node components: DeploymentNode, EnvironmentNode, ProviderNode, ResourceNode, and SystemNode.
    • Launched VisualizePage component for rendering visualization pages.
    • Enhanced TargetDrawer with direct links to visualization pages.
    • Added new relationships procedure for querying resource connections.
  • Improvements

    • Enhanced layout management in React Flow with new layout hooks and configuration options.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 17, 2024

Walkthrough

This pull request introduces several new React components and enhancements to the visualization of resource relationships within a flow diagram. Key additions include the DepEdge, ResourceVisualizationDiagram, and various node components (e.g., DeploymentNode, EnvironmentNode, ProviderNode, ResourceNode, SystemNode). Additionally, new utility functions for creating edges and updates to the database schema to support resource-provider relationships are implemented. The changes enhance the overall structure and functionality of the application, particularly in how resources and their relationships are visually represented.

Changes

File Change Summary
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/DepEdge.tsx - Added DepEdge component for rendering custom edges in a flow diagram, processing props for path calculation and label rendering.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx - Introduced ResourceVisualizationDiagram and ResourceVisualizationDiagramProvider components for visualizing resource relationships.
- Added type ResourceVisualizationDiagramProps and enum NodeType.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/edges.ts - Created utility functions for generating edges between resources and environments, systems, and deployments.
- Added types for Provider and System.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx - Added DeploymentNode component for visual representation of deployment nodes in the flow diagram.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx - Added EnvironmentNode component for visual representation of environment nodes in the flow diagram.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx - Introduced ProviderNode, ProviderIcon, and ProviderLabel components for visualizing provider nodes.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx - Added ResourceNode component for representing resource nodes in the visualization.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/SystemNode.tsx - Introduced SystemNode component for visual representation of system nodes in the flow diagram.
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx - Added VisualizePage component for rendering the visualization page based on resource ID.
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx - Modified TargetDrawer to include dynamic routing to visualization page using workspaceSlug.
packages/db/src/schema/resource-provider.ts - Enhanced schema with new resourceProviderGoogle table and relationships for resource providers.
packages/db/src/schema/resource.ts - Added a new one-to-one relationship for workspace in the resource table.
packages/db/src/schema/system.ts - Added a new relationship for workspace in the system table.
packages/db/src/schema/workspace.ts - Introduced workspaceRelations to establish relationships with resource and system entities.
apps/webservice/src/app/[workspaceSlug]/_components/reactflow/layout.ts - Added LayoutConfig type and useLayoutAndFitView hook for managing layout in React Flow.
packages/api/src/router/resources.ts - Introduced relationships procedure in resourceRouter for querying resource relationships.

Possibly related PRs

Suggested reviewers

  • jsbroks

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.

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

🧹 Outside diff range and nitpick comments (29)
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx (2)

6-10: Consider adding error boundaries for robust error handling.

Since this is an async component fetching data, it would be beneficial to wrap it with an error boundary to gracefully handle runtime errors and provide a better user experience.

Example implementation:

import { ErrorBoundary } from '~/components/ErrorBoundary';

export default async function VisualizePage({
  params: { targetId },
}: {
  params: { targetId: string };
}) {
  return (
    <ErrorBoundary fallback={<div>Error loading visualization</div>}>
      {/* existing component content */}
    </ErrorBoundary>
  );
}

16-21: Consider adding loading state handling.

Since this is an async component, it would be beneficial to show a loading state while data is being fetched. Next.js provides built-in loading.tsx for this purpose.

Create a loading.tsx in the same directory:

// loading.tsx
export default function Loading() {
  return <div>Loading resource visualization...</div>;
}
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx (2)

6-7: Consider enhancing type safety and prop naming

The component could benefit from explicit type definitions and more descriptive parameter naming.

Consider this improvement:

-export const DeploymentNode: React.FC<NodeProps> = (node) => {
-  const { data } = node;
+type DeploymentNodeData = {
+  label: string;
+  // Add other expected data properties
+};
+
+export const DeploymentNode: React.FC<NodeProps<DeploymentNodeData>> = (props) => {
+  const { data } = props;

17-35: Improve style consistency and reduce duplication

The Handle components mix inline styles with Tailwind classes and contain duplicate styling.

Consider consolidating the styles:

+const HANDLE_STYLES = {
+  base: "h-2 w-2 rounded-full border border-amber-500",
+  common: {
+    background: colors.neutral[800],
+  },
+  bottom: {
+    bottom: "0",
+    left: "50%",
+    transform: "translate(-50%, 50%)",
+  },
+} as const;

-      <Handle
-        type="target"
-        className="h-2 w-2 rounded-full border border-amber-500"
-        style={{
-          background: colors.neutral[800],
-        }}
-        position={Position.Top}
-      />
+      <Handle
+        type="target"
+        className={HANDLE_STYLES.base}
+        style={HANDLE_STYLES.common}
+        position={Position.Top}
+      />
       <Handle
         type="source"
-        className="h-2 w-2 rounded-full border border-amber-500"
-        style={{
-          bottom: "0",
-          left: "50%",
-          transform: "translate(-50%, 50%)",
-          background: colors.neutral[800],
-        }}
+        className={HANDLE_STYLES.base}
+        style={{...HANDLE_STYLES.common, ...HANDLE_STYLES.bottom}}
         position={Position.Bottom}
       />
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (4)

1-5: Optimize tailwindcss colors import for better tree-shaking

Instead of importing all colors, import only the specific colors needed (green and neutral).

-import colors from "tailwindcss/colors";
+import { green, neutral } from "tailwindcss/colors";

6-7: Add type safety for the data prop

Consider defining an interface for the expected shape of the data prop to improve type safety and documentation.

+interface EnvironmentNodeData {
+  label: string;
+  // Add other expected properties
+}
+
-export const EnvironmentNode: React.FC<NodeProps> = (node) => {
+export const EnvironmentNode: React.FC<NodeProps<EnvironmentNodeData>> = (node) => {
   const { data } = node;

9-16: Consider improving layout flexibility

The current implementation has some potential layout concerns:

  1. Fixed width of 250px might truncate long labels
  2. Small text sizes (text-xs, text-sm) might affect readability

Consider these improvements:

-<div className="relative flex w-[250px] flex-col gap-2 rounded-md border border-green-600 bg-green-900/30 px-4 py-3">
+<div className="relative flex min-w-[250px] max-w-[350px] flex-col gap-2 rounded-md border border-green-600 bg-green-900/30 px-4 py-3">
   <div className="flex items-center gap-2">
     <IconPlant className="h-4 w-4 text-green-500" />
-    <span className="text-xs">Environment</span>
+    <span className="text-sm font-medium">Environment</span>
   </div>
-  <div className="text-sm">{data.label}</div>
+  <div className="text-sm truncate" title={data.label}>{data.label}</div>

17-35: Improve style consistency and reusability

The Handle components mix Tailwind classes with inline styles. Consider extracting common styles and using Tailwind consistently.

+const handleCommonClasses = "h-2 w-2 rounded-full border border-green-500 bg-neutral-800";
+
 <Handle
   type="target"
-  className="h-2 w-2 rounded-full border border-green-500"
-  style={{
-    background: colors.neutral[800],
-  }}
+  className={handleCommonClasses}
   position={Position.Top}
 />
 <Handle
   type="source"
-  className="h-2 w-2 rounded-full border border-green-500"
+  className={`${handleCommonClasses} translate-y-1/2 -bottom-px left-1/2 -translate-x-1/2`}
-  style={{
-    bottom: "0",
-    left: "50%",
-    transform: "translate(-50%, 50%)",
-    background: colors.neutral[800],
-  }}
   position={Position.Bottom}
 />
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/SystemNode.tsx (4)

1-5: Consider reordering imports following common conventions.

Consider organizing imports in this order:

  1. External libraries (reactflow)
  2. UI libraries (@tabler/icons-react)
  3. Style/configuration imports (tailwindcss/colors)
-import type { NodeProps } from "reactflow";
-import { IconCategory } from "@tabler/icons-react";
-import { Handle, Position } from "reactflow";
-import colors from "tailwindcss/colors";
+import { Handle, Position } from "reactflow";
+import type { NodeProps } from "reactflow";
+import { IconCategory } from "@tabler/icons-react";
+import colors from "tailwindcss/colors";

6-7: Enhance type safety for data prop.

Consider defining a specific type for the data prop to make it clear what properties are expected.

+interface SystemNodeData {
+  label: string;
+}
+
-export const SystemNode: React.FC<NodeProps> = (node) => {
+export const SystemNode: React.FC<NodeProps<SystemNodeData>> = (node) => {
   const { data } = node;

9-16: Consider making the component more configurable and i18n-ready.

A few suggestions to improve flexibility:

  1. Make the width configurable through props
  2. Extract the "System" text for internationalization
  3. Consider making the icon configurable
+interface SystemNodeData {
+  label: string;
+  width?: string;
+  systemLabel?: string;
+  icon?: React.ComponentType<{ className?: string }>;
+}
+
 export const SystemNode: React.FC<NodeProps<SystemNodeData>> = (node) => {
-  const { data } = node;
+  const { data: { label, width = "250px", systemLabel = "System", icon: Icon = IconCategory } } = node;
   return (
     <>
-      <div className="relative flex w-[250px] flex-col gap-2 rounded-md border border-neutral-600 bg-neutral-900/30 px-4 py-3">
+      <div className={`relative flex w-[${width}] flex-col gap-2 rounded-md border border-neutral-600 bg-neutral-900/30 px-4 py-3`}>
         <div className="flex items-center gap-2">
-          <IconCategory className="h-4 w-4 text-neutral-500" />
-          <span className="text-xs">System</span>
+          <Icon className="h-4 w-4 text-neutral-500" />
+          <span className="text-xs">{systemLabel}</span>
         </div>
-        <div className="text-sm">{data.label}</div>
+        <div className="text-sm">{label}</div>

17-35: Improve handle styling consistency and reusability.

Consider extracting handle styles to a common configuration and using Tailwind classes for transforms.

+const handleStyles = {
+  className: "h-2 w-2 rounded-full border border-neutral-500",
+  style: { background: colors.neutral[800] },
+};
+
 export const SystemNode: React.FC<NodeProps<SystemNodeData>> = (node) => {
   // ...
   return (
     <>
       {/* ... */}
       <Handle
         type="target"
-        className="h-2 w-2 rounded-full border border-neutral-500"
-        style={{
-          background: colors.neutral[800],
-        }}
+        {...handleStyles}
         position={Position.Top}
       />
       <Handle
         type="source"
-        className="h-2 w-2 rounded-full border border-neutral-500"
+        {...handleStyles}
+        className={`${handleStyles.className} -bottom-1 left-1/2 -translate-x-1/2 translate-y-1/2`}
-        style={{
-          bottom: "0",
-          left: "50%",
-          transform: "translate(-50%, 50%)",
-          background: colors.neutral[800],
-        }}
         position={Position.Bottom}
       />
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/DepEdge.tsx (2)

16-24: Consider simplifying the getBezierPath call.

The function parameters can be passed directly from the destructured props without re-destructuring.

-  const [edgePath, labelX, labelY] = getBezierPath({
-    sourceX,
-    sourceY,
-    sourcePosition,
-    targetX,
-    targetY,
-    targetPosition,
-  });
+  const [edgePath, labelX, labelY] = getBezierPath({
+    sourceX, sourceY, sourcePosition,
+    targetX, targetY, targetPosition,
+  });

27-48: Consider CSS improvements and enhanced documentation.

While the implementation is correct, consider these improvements:

  1. Extract transform calculations into CSS variables
  2. Add more specific documentation about what interactions are enabled by pointerEvents: "all"
       <EdgeLabelRenderer>
         <div
           style={{
-            transform: `translate(-50%, -50%) translate(${labelX}px,${labelY}px)`,
+            '--label-x': `${labelX}px`,
+            '--label-y': `${labelY}px`,
+            transform: 'translate(-50%, -50%) translate(var(--label-x), var(--label-y))',
             // everything inside EdgeLabelRenderer has no pointer events by default
-            // if you have an interactive element, set pointer-events: all
+            // setting pointer-events: all enables clicking and hovering on the label
             pointerEvents: "all",
           }}
           className="nodrag nopan absolute z-10 text-sm"
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/edges.ts (4)

9-12: Consider improving type safety and theme flexibility.

The marker styling could benefit from:

  1. Type definition for better type safety
  2. Theme-aware color implementation for better customization
+import { EdgeMarker } from 'reactflow';
+
+const markerEnd: EdgeMarker = {
-const markerEnd = {
   type: MarkerType.Arrow,
-  color: colors.neutral[700],
+  color: 'var(--edge-marker-color, ${colors.neutral[700]})',
 };

14-25: Consider using a more descriptive edge label.

The label "in" might be too terse. Consider using a more descriptive label like "deployed in" or "exists in" to improve clarity for users.

     style: { stroke: colors.neutral[700] },
     markerEnd,
-    label: "in",
+    label: "deployed in",

44-54: Consider extracting common edge creation logic.

There's duplicate code between this and other edge creation functions. Consider extracting the common edge creation logic into a helper function.

+const createEdge = (source: string, target: string, label: string) => ({
+  id: `${source}-${target}`,
+  source,
+  target,
+  style: { stroke: colors.neutral[700] },
+  markerEnd,
+  label,
+});
+
 export const createEdgesFromSystemsToDeployments = (systems: System[]) =>
   systems.flatMap((system) =>
     system.deployments.map((deployment) => ({
-      id: `${system.id}-${deployment.id}`,
-      source: system.id,
-      target: deployment.id,
-      style: { stroke: colors.neutral[700] },
-      markerEnd,
-      label: "deploys",
+      ...createEdge(system.id, deployment.id, "deploys")
     })),
   );

56-69: Consider using explicit null check for better readability.

While the current null check works, an explicit check would be more readable and consistent with TypeScript best practices.

 export const createEdgeFromProviderToResource = (
   provider: Provider | null,
   resource: SCHEMA.Resource,
 ) =>
-  provider != null
+  provider === null
+    ? null
     ? {
         id: `${provider.id}-${resource.id}`,
         source: provider.id,
         target: resource.id,
         style: { stroke: colors.neutral[700] },
         markerEnd,
         label: "discovered",
-      }
-    : null;
+      };
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx (3)

6-6: Consider renaming the imported icon component.

The alias ResourceIcon for TargetIcon might be confusing as it suggests a different semantic meaning. Consider either:

  1. Using the original name if it represents a target
  2. Creating a dedicated ResourceIcon component if it truly represents a resource

39-58: Reduce code duplication in Handle components.

The styling logic is duplicated between the target and source Handle components. Consider extracting the common styles into a variable.

+  const handleStyles = cn(
+    "h-2 w-2 rounded-full border border-neutral-500 bg-neutral-800",
+    isKubernetes && "border-blue-500/70",
+    isTerraform && "border-purple-500/70",
+    isSharedCluster && "border-blue-500/70",
+  );

   <Handle
     type="target"
-    className={cn(
-      "h-2 w-2 rounded-full border border-neutral-500 bg-neutral-800",
-      isKubernetes && "border-blue-500/70",
-      isTerraform && "border-purple-500/70",
-      isSharedCluster && "border-blue-500/70",
-    )}
+    className={handleStyles}
     position={Position.Top}
   />
   <Handle
     type="source"
-    className={cn(
-      "h-2 w-2 rounded-full border border-neutral-500 bg-neutral-800",
-      isKubernetes && "border-blue-500/70",
-      isTerraform && "border-purple-500/70",
-      isSharedCluster && "border-blue-500/70",
-    )}
+    className={handleStyles}
     position={Position.Bottom}
   />

24-30: Consider making the node width configurable.

The hardcoded width of w-[250px] might not be flexible enough for different content lengths or responsive layouts.

Consider either:

  1. Making it a prop with a default value
  2. Using a relative width with max-width constraints
  3. Using a CSS variable for easy customization
-          "flex w-[250px] flex-col gap-2 rounded-md border bg-neutral-900 px-4 py-3",
+          "flex w-full max-w-[250px] flex-col gap-2 rounded-md border bg-neutral-900 px-4 py-3",
packages/db/src/schema/resource-provider.ts (2)

68-76: Consider adding uniqueness constraints for project IDs

While the relationship is correctly defined, consider adding a unique constraint on projectIds to prevent duplicate project configurations within the same resource provider. This would ensure data consistency and prevent potential issues with multiple configurations for the same Google project.

Here's how you could modify the table definition:

export const resourceProviderGoogle = pgTable(
  "resource_provider_google",
  {
    id: uuid("id").primaryKey().defaultRandom(),
    resourceProviderId: uuid("resource_provider_id")
      .notNull()
      .references(() => resourceProvider.id, { onDelete: "cascade" }),
    projectIds: text("project_ids").array().notNull(),
    importGke: boolean("import_gke").notNull().default(false),
    importNamespaces: boolean("import_namespaces").notNull().default(false),
    importVCluster: boolean("import_vcluster").notNull().default(false),
  },
+ (t) => ({
+   uniqProjectIds: uniqueIndex().on(t.resourceProviderId, t.projectIds),
+ }),
);

Line range hint 32-76: Well-structured schema design for resource visualization

The schema design effectively supports resource relationship visualization with:

  1. Clear separation between generic resource providers and Google-specific configurations
  2. Proper cascading delete behavior to maintain referential integrity
  3. Flexible relationship modeling that allows for future extension to other cloud providers

Consider documenting these relationship patterns for future maintainers who might add support for other cloud providers.

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx (2)

17-24: Consider enhancing component reusability.

The component works well but could be more flexible with these improvements:

  1. Accept a className prop for custom styling
  2. Move color values to a theme configuration for consistency
-export const ProviderIcon: React.FC<{ node: ProviderNodeProps }> = ({
+export const ProviderIcon: React.FC<{
+  node: ProviderNodeProps;
+  className?: string;
+}> = ({
   node,
+  className,
 }) => {
   const { google } = node.data;
   if (google != null)
-    return <IconBrandGoogleFilled className="h-4 w-4 text-red-500" />;
+    return <IconBrandGoogleFilled className={cn("h-4 w-4 text-red-500", className)} />;
-  return <IconCube className="h-4 w-4 text-neutral-500" />;
+  return <IconCube className={cn("h-4 w-4 text-neutral-500", className)} />;
 };

26-31: Align styling approach with ProviderIcon component.

For consistency with the previous component, consider:

  1. Making colors configurable
  2. Adding className prop for custom styling
-const ProviderLabel: React.FC<{ node: ProviderNodeProps }> = ({ node }) => {
+const ProviderLabel: React.FC<{
+  node: ProviderNodeProps;
+  className?: string;
+}> = ({ node, className }) => {
   const { google } = node.data;
   if (google != null)
-    return <span className="text-xs text-red-500">Google Provider</span>;
+    return <span className={cn("text-xs text-red-500", className)}>Google Provider</span>;
-  return <span className="text-xs">Resource Provider</span>;
+  return <span className={cn("text-xs", className)}>Resource Provider</span>;
 };
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (1)

128-138: Consider enhancing the visualization link with error handling and accessibility improvements

While the implementation is solid, consider these enhancements:

  1. Add error handling for cases where the visualization might not be available
  2. Enhance accessibility by adding an aria-label to better describe the action
 <Link
   href={`/${workspaceSlug}/targets/${target.id}/visualize`}
   className={buttonVariants({
     variant: "outline",
     size: "sm",
     className: "gap-1",
   })}
+  aria-label={`Visualize relationships for ${target.name}`}
 >
   <IconTopologyStar3 className="h-3 w-3" /> Visualize
 </Link>
packages/db/src/schema/resource.ts (1)

70-73: Consider enhancing relationship metadata for visualization.

Given that this PR focuses on resource relationships visualization, you might want to consider adding metadata fields to the resourceRelationship table to support richer visualizations. Some suggestions:

  • Add a metadata JSONB column for storing visualization-specific data (e.g., custom colors, line styles)
  • Add a description field to provide context about the relationship
  • Consider adding a strength or weight field if you plan to implement hierarchical layouts

These fields could enhance the visualization capabilities without requiring schema changes later.

Also applies to: 200-220

packages/api/src/router/target.ts (1)

389-392: Improve type safety in provider.google handling

The current implementation uses the non-null assertion operator (!) which could lead to runtime errors. Consider using optional chaining or explicit null checks.

- google:
-   resource.provider.google.length > 0
-     ? resource.provider.google[0]!
-     : null,
+ google:
+   resource.provider.google?.[0] ?? null,
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (1)

151-153: Remove console.log statements from production code

Including console.log in your code can lead to unnecessary console clutter and potential performance issues in production environments. Consider removing it or implementing a logging mechanism that can be disabled in production builds.

Apply this diff to remove the console log statement:

 const onNodeClick: NodeMouseHandler = (event, node) => {
-  console.log(node);
 };
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 988a809 and ace764b.

📒 Files selected for processing (16)
  • apps/pty-proxy/src/config.ts (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/DepEdge.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/edges.ts (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/SystemNode.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (3 hunks)
  • packages/api/src/router/target.ts (2 hunks)
  • packages/db/src/schema/resource-provider.ts (2 hunks)
  • packages/db/src/schema/resource.ts (1 hunks)
  • packages/db/src/schema/system.ts (1 hunks)
  • packages/db/src/schema/workspace.ts (3 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/DepEdge.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#165
File: apps/webservice/src/app/[workspaceSlug]/_components/relationships/RelationshipsDiagramDependencies.tsx:185-189
Timestamp: 2024-11-12T09:55:38.456Z
Learning: In `RelationshipsDiagramDependencies.tsx`, when using React Flow's `BaseEdge`, the `markerStart` prop may intentionally use the `markerEnd` value to flip the arrow direction because 'source depends on target' logic is applied. These are intrinsic attributes of the React Flow `BaseEdge` component and cannot be renamed or changed.
🔇 Additional comments (26)
apps/pty-proxy/src/config.ts (2)

15-15: Clarify the relationship to visualization features

Given that this PR is about "Init resource relationships visualization", could you explain how this auth URL change relates to the visualization features? This seems like an unrelated change that might have been included accidentally.

Let's check the visualization-related changes:

#!/bin/bash
# Look for visualization-related files
fd -e tsx -e ts visualize

# Check for auth-related code in visualization components
rg "auth" $(fd -e tsx -e ts visualize)

15-15: ⚠️ Potential issue

Breaking Change: Verify the removal of auth session endpoint path

The default AUTH_URL has been changed from "http://localhost:3000/api/auth/session" to "http://localhost:3000", removing the specific auth endpoint path. This could break authentication if components expect the full session URL.

Let's verify the usage of this auth URL:

✅ Verification successful

No Breaking Change: Auth URL Path is Correctly Appended

The removal of "/api/auth/session" from the default AUTH_URL is not a breaking change. The auth.ts file explicitly appends "/api/auth/session" when making the request:

const res = await fetch(`${env.AUTH_URL}/api/auth/session`, options);

This means the authentication endpoint path is properly handled in the code, regardless of whether it's included in the AUTH_URL environment variable.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to auth session endpoint
rg -l "api/auth/session" --type ts --type tsx

# Search for direct usage of AUTH_URL
rg "AUTH_URL" --type ts --type tsx -C 3

Length of output: 50436


Script:

#!/bin/bash
# Search for direct usage of AUTH_URL in pty-proxy
rg "AUTH_URL" --type ts --type tsx -C 3 apps/pty-proxy/

# Search for session endpoint usage
rg "api/auth/session" --type ts --type tsx -C 3 apps/pty-proxy/

Length of output: 175


Script:

#!/bin/bash
# Search for direct usage of AUTH_URL in pty-proxy without file type filters
rg "AUTH_URL" -C 3 apps/pty-proxy/

# Search for session endpoint usage without file type filters
rg "api/auth/session" -C 3 apps/pty-proxy/

Length of output: 1685

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx (2)

1-4: LGTM! Imports are well-organized and necessary.


4-4: Verify the ResourceVisualizationDiagramProvider implementation.

Let's ensure that the imported provider component is properly implemented and exported.

✅ Verification successful

The ResourceVisualizationDiagramProvider is properly implemented and exported

The component is correctly implemented in ResourceVisualizationDiagram.tsx as a React functional component that wraps ResourceVisualizationDiagram with ReactFlowProvider. It's properly exported and used in the visualization page.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the ResourceVisualizationDiagramProvider is properly exported
ast-grep --pattern 'export { ResourceVisualizationDiagramProvider }' || \
ast-grep --pattern 'export const ResourceVisualizationDiagramProvider'

# Check the implementation
rg -A 10 'ResourceVisualizationDiagramProvider.*=.*' || \
rg -A 10 'function ResourceVisualizationDiagramProvider.*'

Length of output: 186


Script:

#!/bin/bash
# Let's try to find the file first and then examine its contents
fd ResourceVisualizationDiagram --exec cat {}

# As a backup, let's also check for any references to this component
rg "ResourceVisualizationDiagramProvider" -B 2 -A 2

Length of output: 7224

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/DeploymentNode.tsx (2)

1-5: LGTM: Clean and necessary imports

The imports are well-organized and properly typed, including only the necessary dependencies for the component's functionality.


1-38: Verify integration with the visualization system

Since this component is part of a larger visualization system, ensure it integrates properly with other node types and the edge components.

✅ Verification successful

DeploymentNode properly integrates with visualization system

The component follows the established patterns seen in other node components:

  • Consistent structure with other nodes (EnvironmentNode, ResourceNode)
  • Proper Handle components for top and bottom connections
  • Consistent styling approach using color themes
  • Correct implementation of NodeProps and reactflow interfaces
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify integration with other visualization components

# Check for other node types and their styling consistency
echo "Checking for other node types..."
fd --type f --glob '*Node.tsx' "$(dirname "$0")"

# Check for edge definitions that might connect to this node
echo "Checking for edge definitions..."
rg --type typescript "DeploymentNode" -A 5

Length of output: 1614


Script:

#!/bin/bash
# Let's examine the other node components in the same directory for styling consistency
echo "Checking EnvironmentNode styling..."
cat "/home/jailuser/git/apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx"

echo -e "\nChecking ResourceNode styling..."
cat "/home/jailuser/git/apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx"

# Let's also check for any flow-related configuration or edge definitions
echo -e "\nChecking for flow configurations..."
rg -g "*.{ts,tsx}" "reactflow.*Handle" "apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/"

Length of output: 3749

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/EnvironmentNode.tsx (1)

6-38: Verify integration with ResourceVisualizationDiagram

The component looks well-structured, but let's verify its integration with the broader visualization system.

✅ Verification successful

EnvironmentNode is properly integrated with the visualization system

The component is correctly integrated within the React Flow visualization system:

  • Properly imported and registered in ResourceVisualizationDiagram.tsx as a node type with the correct NodeType.Environment key
  • Follows the same structure as other node components in the system
  • Correctly implements the required React Flow node interfaces with proper Handle components for connections
  • Used consistently across different visualization contexts (resource visualization and flow diagrams)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify proper integration of EnvironmentNode in the visualization system

# Check if EnvironmentNode is properly registered in the node types
rg -A 5 "nodeTypes.*EnvironmentNode" apps/webservice/src/app

# Check for any custom edge definitions related to EnvironmentNode
rg "source.*Environment.*target|target.*Environment.*source" apps/webservice/src/app

Length of output: 149


Script:

#!/bin/bash
# Let's try a broader search to understand the visualization system structure

# Find files containing EnvironmentNode references
rg "EnvironmentNode" apps/webservice/src/app

# Look for React Flow node type definitions or mappings
rg -A 5 "nodeTypes.*=|getNodeTypes" apps/webservice/src/app

# Search for files that might contain node registration logic
fd "diagram|flow|graph" apps/webservice/src/app

# Look for the parent component that might be using EnvironmentNode
ast-grep --pattern 'import { EnvironmentNode } from "$_"'

Length of output: 12556

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/SystemNode.tsx (1)

1-38: Overall implementation looks good!

The SystemNode component is well-structured and implements the required functionality correctly. The suggested improvements are optional enhancements that could make the component more maintainable and reusable.

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/DepEdge.tsx (2)

1-15: LGTM! Component declaration and imports are well-structured.

The component properly leverages React Flow's type system and includes all necessary imports. The default style initialization is a good practice.


31-31: Design note about markerEnd usage.

The use of markerEnd for arrow direction is intentional and follows React Flow's convention where 'source depends on target' logic is applied.

packages/db/src/schema/system.ts (2)

51-57: LGTM! The workspace relationship is correctly defined.

The one-to-one relationship is properly configured with appropriate field mappings and follows Drizzle ORM best practices.


51-57: Verify the bi-directional relationship setup

Please ensure that:

  1. The corresponding relationship is defined in workspace.ts
  2. Existing data has valid workspace associations
✅ Verification successful

Let me gather more information to verify the relationship setup.


Based on the results, I can now provide the final response:

Bi-directional relationship is correctly configured

The verification confirms that:

  1. The workspace relation is properly defined with systems: many(system) in workspace.ts
  2. The system table has a proper foreign key constraint to workspace table with ON DELETE no action, ensuring data integrity
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the bi-directional relationship setup and data integrity

# Check if workspace.ts defines the reverse relationship
echo "Checking workspace.ts for system relationship..."
ast-grep --pattern 'export const workspaceRelations = relations(workspace, ({ $_ }) => ({
  $$$
  systems: $_($_, {
    $$$
  }),
  $$$
}))'

# Check if all systems have valid workspace associations
echo "Checking for systems without workspace associations..."
rg -l "CREATE TABLE.*system" | while read -r file; do
  echo "Found system table definition in: $file"
done

Length of output: 592


Script:

#!/bin/bash
# Check the workspace relations definition
echo "Checking workspace.ts content..."
rg -A 10 "workspaceRelations.*=.*relations" packages/db/src/schema/

# Check the system table definition for workspace reference
echo "Checking system table definition..."
cat packages/db/drizzle/0000_busy_firebird.sql

Length of output: 40388

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/edges.ts (3)

1-7: LGTM! Clean imports and well-defined types.

The imports are specific and the Provider type is properly defined with nullable properties.


27-30: LGTM! Well-structured type definition.

The System type properly extends the base schema type with strongly-typed arrays.


32-42: LGTM! Efficient implementation with clear relationship labeling.

The function effectively handles the nested structure using flatMap and provides clear relationship labeling.

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ResourceNode.tsx (1)

8-14: LGTM! Well-structured type definition.

The ResourceNodeProps type is well-defined with clear property names and appropriate types. The extension of NodeProps from reactflow is correctly implemented.

packages/db/src/schema/resource-provider.ts (1)

32-35: LGTM: Relationship is correctly defined

The many-to-many relationship between resourceProvider and resourceProviderGoogle is properly established, allowing multiple Google configurations per resource provider.

apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/nodes/ProviderNode.tsx (2)

1-15: LGTM! Well-structured imports and type definition.

The imports are properly organized and the ProviderNodeProps type is well-defined with appropriate typing for all fields.


1-72: Consider adding ARIA attributes for accessibility.

The component could benefit from improved accessibility support for users relying on screen readers.

Consider adding these accessibility improvements:

 <div
   className={cn(
     "relative flex w-[250px] flex-col gap-2 rounded-md border border-green-600 bg-green-900/30 px-4 py-3",
     node.data.google != null && "border-red-600 bg-red-900/30",
   )}
+  role="button"
+  aria-label={`${data.google ? 'Google' : 'Resource'} Provider: ${data.label}`}
 >
packages/db/src/schema/workspace.ts (2)

2-2: LGTM: Import statements are well-organized

The new imports are correctly placed and necessary for defining the workspace relationships.

Also applies to: 15-16


51-54: Verify bidirectional relationships and migration strategy

The workspace relations are correctly defined, but please ensure:

  1. Corresponding relations are defined in resource.ts and system.ts
  2. Cascade delete behavior is consistent across related entities
  3. Database migration strategy is in place for these new relationships

Let's verify the related schema definitions:

✅ Verification successful

Based on the code analysis, I can now generate the final response:

Bidirectional relationships are properly defined

The workspace-resource and workspace-system relationships are correctly implemented:

  • In workspace.ts: The workspace defines many-to-many relationships with resources and systems
  • In resource.ts: The resource defines a one-to-one relationship with workspace via workspaceId foreign key with cascade delete
  • In system.ts: The system defines a one-to-one relationship with workspace via workspaceId foreign key with cascade delete

The cascade delete behavior is consistently defined across all entities, ensuring data integrity:

  • Resources: onDelete: "cascade" on workspaceId
  • Systems: onDelete: "cascade" on workspaceId
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for corresponding relationship definitions in resource.ts and system.ts
echo "Checking resource.ts for workspace relation:"
rg -A 5 "relations.*workspace" packages/db/src/schema/resource.ts

echo "Checking system.ts for workspace relation:"
rg -A 5 "relations.*workspace" packages/db/src/schema/system.ts

# Check for any existing migrations
echo "Checking for related migrations:"
fd -e sql . packages/db/migrations

Length of output: 549


Script:

#!/bin/bash
# Let's first check if the files exist and their locations
echo "Searching for schema files:"
fd -t f "resource.ts|system.ts|workspace.ts" 

# Then let's examine the content of these files
echo -e "\nChecking resource.ts content:"
fd -t f "resource.ts" --exec cat {}

echo -e "\nChecking system.ts content:"
fd -t f "system.ts" --exec cat {}

# Let's also check for migrations directory
echo -e "\nSearching for migrations directory:"
fd -t d "migrations"

Length of output: 32230

apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (3)

5-5: LGTM: Clean import addition

The addition of useParams import is properly organized alongside the existing navigation imports.


37-37: LGTM: Proper hook usage with type safety

The useParams hook is correctly implemented with proper TypeScript type checking for the workspaceSlug parameter.


128-138: Verify the visualization route handling

Ensure that the visualization route is properly set up to handle the provided parameters.

✅ Verification successful

Route exists and is properly handling parameters

The visualization route is correctly implemented at apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx. The page component:

  • Properly extracts the targetId parameter
  • Fetches the required resource and its relationships
  • Handles the not-found case appropriately
  • Renders a visualization diagram component with the necessary data
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the visualization page route exists and handles the parameters correctly
fd -e tsx -e ts "page" "app/\[workspaceSlug\]/targets/\[targetId\]/visualize"

# Check for any route parameter validation
rg -l "params.*targetId.*workspaceSlug" "app/\[workspaceSlug\]/targets/\[targetId\]/visualize"

Length of output: 407


Script:

#!/bin/bash
# Let's first check if the visualization route exists anywhere in the codebase
fd -e tsx -e ts "page" "apps/webservice/src"

# Search for any route handling related to target visualization
rg -l "targets/\[.*\]/visualize" "apps/webservice/src"

# Search for any components or logic related to target visualization
rg -l "visualize.*target" "apps/webservice/src"

Length of output: 4586


Script:

#!/bin/bash
# Let's check the content of the visualize page since we found it in the first search
cat "apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx"

# Also check for any related components or utilities
rg -g "!*.test.*" -g "!*.spec.*" "visualize" "apps/webservice/src/app/[workspaceSlug]/(targets)/targets/"

Length of output: 829

packages/db/src/schema/resource.ts (1)

70-73: LGTM! The workspace relation is properly defined.

The new one-to-one relationship between resource and workspace is correctly implemented, using proper field references and maintaining consistency with the existing schema constraints.

Let's verify the relationship definition is consistent with other related schemas:

✅ Verification successful

The workspace relation is correctly defined and consistent with the schema

The verification confirms:

  • The resource table has a proper workspaceId field (as seen in the relations)
  • The workspace relation in resourceRelations correctly maps resource.workspaceId to workspace.id
  • The workspace table has a corresponding many relation to resources in workspaceRelations
  • No conflicting unique constraints were found
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify workspace relationship consistency across schema files

# Check for corresponding relationship in workspace schema
echo "Checking workspace schema for corresponding resource relationship..."
rg -A 5 "relations.*workspace.*" packages/db/src/schema/workspace.ts

# Verify no conflicting unique constraints
echo "Checking for any conflicting unique constraints..."
rg "uniqueIndex.*workspace.*resource" packages/db/src/schema/

Length of output: 577


Script:

#!/bin/bash
# Let's verify the schema definitions more thoroughly

# Check resource table definition for workspaceId field and constraints
echo "Checking resource table definition..."
rg -B 5 -A 10 "export const resource = pgTable" packages/db/src/schema/resource.ts

# Check all relations defined in resource schema
echo "Checking resource relations..."
rg -B 2 -A 10 "export const resourceRelations" packages/db/src/schema/resource.ts

# Check workspace table definition
echo "Checking workspace table definition..."
rg -B 5 -A 10 "export const workspace = pgTable" packages/db/src/schema/workspace.ts

Length of output: 2422

packages/api/src/router/target.ts (1)

12-12: LGTM: Import addition is appropriate

The isNotNull import is correctly placed and properly utilized in the new relationships procedure.

Comment on lines 11 to 14
const resource = await api.resource.byId(targetId);
if (resource == null) return notFound();
const relationships = await api.resource.relationships(resource.id);
if (relationships == null) return notFound();
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

Optimize API calls and enhance error handling.

  1. Consider using Promise.all to parallelize the API calls since the relationships fetch doesn't depend on the resource data beyond the ID.
  2. Add proper error handling for API failures beyond just "not found" scenarios.

Here's a suggested implementation:

try {
  const [resource, relationships] = await Promise.all([
    api.resource.byId(targetId),
    api.resource.relationships(targetId)
  ]);
  
  if (!resource || !relationships) {
    return notFound();
  }
  
  return (
    <ResourceVisualizationDiagramProvider
      resource={resource}
      relationships={relationships}
    />
  );
} catch (error) {
  console.error('Failed to fetch visualization data:', error);
  throw error; // Let the error boundary handle it
}

Comment on lines +8 to +16
return (
<>
<div className="relative flex w-[250px] flex-col gap-2 rounded-md border border-amber-600 bg-amber-900/30 px-4 py-3">
<div className="flex items-center gap-2">
<IconShip className="h-4 w-4 text-amber-500" />
<span className="text-xs">Deployment</span>
</div>
<div className="text-sm">{data.label}</div>
</div>
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

Enhance maintainability and error handling

Consider the following improvements:

  1. Extract color and dimension constants
  2. Add error handling for missing data

Here's a suggested implementation:

+const STYLES = {
+  container: "relative flex w-[250px] flex-col gap-2 rounded-md border border-amber-600 bg-amber-900/30 px-4 py-3",
+  header: "flex items-center gap-2",
+  icon: "h-4 w-4 text-amber-500",
+  label: "text-xs",
+  content: "text-sm"
+} as const;

 export const DeploymentNode: React.FC<NodeProps> = (node) => {
   const { data } = node;
+  if (!data?.label) {
+    console.warn('DeploymentNode: Missing required data.label');
+    return null;
+  }
   return (
     <>
-      <div className="relative flex w-[250px] flex-col gap-2 rounded-md border border-amber-600 bg-amber-900/30 px-4 py-3">
-        <div className="flex items-center gap-2">
-          <IconShip className="h-4 w-4 text-amber-500" />
-          <span className="text-xs">Deployment</span>
+      <div className={STYLES.container}>
+        <div className={STYLES.header}>
+          <IconShip className={STYLES.icon} />
+          <span className={STYLES.label}>Deployment</span>
         </div>
-        <div className="text-sm">{data.label}</div>
+        <div className={STYLES.content}>{data.label}</div>
       </div>
📝 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
return (
<>
<div className="relative flex w-[250px] flex-col gap-2 rounded-md border border-amber-600 bg-amber-900/30 px-4 py-3">
<div className="flex items-center gap-2">
<IconShip className="h-4 w-4 text-amber-500" />
<span className="text-xs">Deployment</span>
</div>
<div className="text-sm">{data.label}</div>
</div>
const STYLES = {
container: "relative flex w-[250px] flex-col gap-2 rounded-md border border-amber-600 bg-amber-900/30 px-4 py-3",
header: "flex items-center gap-2",
icon: "h-4 w-4 text-amber-500",
label: "text-xs",
content: "text-sm"
} as const;
export const DeploymentNode: React.FC<NodeProps> = (node) => {
const { data } = node;
if (!data?.label) {
console.warn('DeploymentNode: Missing required data.label');
return null;
}
return (
<>
<div className={STYLES.container}>
<div className={STYLES.header}>
<IconShip className={STYLES.icon} />
<span className={STYLES.label}>Deployment</span>
</div>
<div className={STYLES.content}>{data.label}</div>
</div>

targetPosition,
});

const edgeLabel = capitalCase(String(label).replace("_", " "));
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

Fix underscore replacement to handle multiple occurrences.

The current implementation only replaces the first underscore in the label. Use a regular expression to replace all underscores.

-  const edgeLabel = capitalCase(String(label).replace("_", " "));
+  const edgeLabel = capitalCase(String(label).replace(/_/g, " "));
📝 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
const edgeLabel = capitalCase(String(label).replace("_", " "));
const edgeLabel = capitalCase(String(label).replace(/_/g, " "));

Comment on lines +18 to +20
const isKubernetes = data.version.includes("kubernetes");
const isTerraform = data.version.includes("terraform");
const isSharedCluster = data.kind.toLowerCase().includes("sharedcluster");
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

Strengthen the resource type detection logic.

The current string matching approach using includes() might be fragile:

  1. Case sensitivity might cause issues (only kind is converted to lowercase)
  2. Partial matches could lead to false positives (e.g., "kubernetes-dev" vs "kubernetes")
  3. No validation for undefined/null values

Consider this more robust approach:

-  const isKubernetes = data.version.includes("kubernetes");
-  const isTerraform = data.version.includes("terraform");
-  const isSharedCluster = data.kind.toLowerCase().includes("sharedcluster");
+  const isKubernetes = data.version?.toLowerCase() === "kubernetes";
+  const isTerraform = data.version?.toLowerCase() === "terraform";
+  const isSharedCluster = data.kind?.toLowerCase() === "sharedcluster";

Also, consider defining an enum or constant for these values to prevent typos:

const RESOURCE_TYPES = {
  KUBERNETES: "kubernetes",
  TERRAFORM: "terraform",
  SHARED_CLUSTER: "sharedcluster"
} as const;

Comment on lines +33 to +72
export const ProviderNode: React.FC<ProviderNodeProps> = (node) => {
const { data } = node;
return (
<>
<div
className={cn(
"relative flex w-[250px] flex-col gap-2 rounded-md border border-green-600 bg-green-900/30 px-4 py-3",
node.data.google != null && "border-red-600 bg-red-900/30",
)}
>
<div className="flex items-center gap-2">
<ProviderIcon node={node} />
<ProviderLabel node={node} />
</div>
<div className="text-sm">{data.label}</div>
</div>
<Handle
type="target"
className={cn(
"h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
data.google != null && "border-red-500 bg-red-800",
)}
position={Position.Top}
/>
<Handle
type="source"
className={cn(
"h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
data.google != null && "border-red-500 bg-red-800",
)}
style={{
bottom: "0",
left: "50%",
transform: "translate(-50%, 50%)",
}}
position={Position.Bottom}
/>
</>
);
};
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

Improve code organization and reduce duplication.

The component has several areas for improvement:

  1. Extract duplicate handle styles into a shared constant
  2. Consider extracting handle positioning logic into a utility
  3. Break down the complex JSX structure into smaller components
+const getHandleStyles = (isGoogle: boolean) => cn(
+  "h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
+  isGoogle && "border-red-500 bg-red-800",
+);
+
+const BottomHandlePosition = {
+  bottom: "0",
+  left: "50%",
+  transform: "translate(-50%, 50%)",
+} as const;
+
 export const ProviderNode: React.FC<ProviderNodeProps> = (node) => {
   const { data } = node;
+  const handleStyles = getHandleStyles(data.google != null);
   return (
     <>
       <div
         className={cn(
           "relative flex w-[250px] flex-col gap-2 rounded-md border border-green-600 bg-green-900/30 px-4 py-3",
           node.data.google != null && "border-red-600 bg-red-900/30",
         )}
       >
         <div className="flex items-center gap-2">
           <ProviderIcon node={node} />
           <ProviderLabel node={node} />
         </div>
         <div className="text-sm">{data.label}</div>
       </div>
       <Handle
         type="target"
-        className={cn(
-          "h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
-          data.google != null && "border-red-500 bg-red-800",
-        )}
+        className={handleStyles}
         position={Position.Top}
       />
       <Handle
         type="source"
-        className={cn(
-          "h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
-          data.google != null && "border-red-500 bg-red-800",
-        )}
-        style={{
-          bottom: "0",
-          left: "50%",
-          transform: "translate(-50%, 50%)",
-        }}
+        className={handleStyles}
+        style={BottomHandlePosition}
         position={Position.Bottom}
       />
     </>
   );
 };
📝 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 ProviderNode: React.FC<ProviderNodeProps> = (node) => {
const { data } = node;
return (
<>
<div
className={cn(
"relative flex w-[250px] flex-col gap-2 rounded-md border border-green-600 bg-green-900/30 px-4 py-3",
node.data.google != null && "border-red-600 bg-red-900/30",
)}
>
<div className="flex items-center gap-2">
<ProviderIcon node={node} />
<ProviderLabel node={node} />
</div>
<div className="text-sm">{data.label}</div>
</div>
<Handle
type="target"
className={cn(
"h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
data.google != null && "border-red-500 bg-red-800",
)}
position={Position.Top}
/>
<Handle
type="source"
className={cn(
"h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
data.google != null && "border-red-500 bg-red-800",
)}
style={{
bottom: "0",
left: "50%",
transform: "translate(-50%, 50%)",
}}
position={Position.Bottom}
/>
</>
);
};
const getHandleStyles = (isGoogle: boolean) => cn(
"h-2 w-2 rounded-full border border-green-500 bg-neutral-800",
isGoogle && "border-red-500 bg-red-800",
);
const BottomHandlePosition = {
bottom: "0",
left: "50%",
transform: "translate(-50%, 50%)",
} as const;
export const ProviderNode: React.FC<ProviderNodeProps> = (node) => {
const { data } = node;
const handleStyles = getHandleStyles(data.google != null);
return (
<>
<div
className={cn(
"relative flex w-[250px] flex-col gap-2 rounded-md border border-green-600 bg-green-900/30 px-4 py-3",
node.data.google != null && "border-red-600 bg-red-900/30",
)}
>
<div className="flex items-center gap-2">
<ProviderIcon node={node} />
<ProviderLabel node={node} />
</div>
<div className="text-sm">{data.label}</div>
</div>
<Handle
type="target"
className={handleStyles}
position={Position.Top}
/>
<Handle
type="source"
className={handleStyles}
style={BottomHandlePosition}
position={Position.Bottom}
/>
</>
);
};

Comment on lines 372 to 374
const matchedEnvironments = await Promise.all(
matchedEnvironmentsPromises,
).then((t) => t.filter(isPresent));
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

Add error handling for Promise.all operations

The current implementation doesn't handle potential promise rejections explicitly. Consider adding error handling to gracefully handle failures.

- const matchedEnvironments = await Promise.all(matchedEnvironmentsPromises)
-   .then((t) => t.filter(isPresent));
+ const matchedEnvironments = await Promise.all(matchedEnvironmentsPromises)
+   .then((t) => t.filter(isPresent))
+   .catch((error) => {
+     console.error('Failed to fetch matched environments:', error);
+     throw new TRPCError({
+       code: 'INTERNAL_SERVER_ERROR',
+       message: 'Failed to process environment matches',
+     });
+   });

Also applies to: 380-382

Comment on lines 360 to 378
const matchedSystemsPromises = systems.map(async (s) => {
const matchedEnvironmentsPromises = s.environments.map(async (e) => {
const matchedResource = await ctx.db.query.resource.findFirst({
where: and(
eq(schema.resource.workspaceId, s.workspaceId),
schema.resourceMatchesMetadata(ctx.db, e.resourceFilter),
),
});

return matchedResource != null ? e : null;
});

const matchedEnvironments = await Promise.all(
matchedEnvironmentsPromises,
).then((t) => t.filter(isPresent));
if (matchedEnvironments.length === 0) return null;

return { ...s, environments: matchedEnvironments };
});
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

Consider optimizing database queries to reduce N+1 problems

The current implementation performs database queries in a loop for each environment, which could lead to performance issues with larger datasets. Consider refactoring to use a single query with JOIN operations.

Here's a suggested approach:

- const matchedSystemsPromises = systems.map(async (s) => {
-   const matchedEnvironmentsPromises = s.environments.map(async (e) => {
-     const matchedResource = await ctx.db.query.resource.findFirst({
-       where: and(
-         eq(schema.resource.workspaceId, s.workspaceId),
-         schema.resourceMatchesMetadata(ctx.db, e.resourceFilter),
-       ),
-     });
-     return matchedResource != null ? e : null;
-   });
-   const matchedEnvironments = await Promise.all(matchedEnvironmentsPromises)
-     .then((t) => t.filter(isPresent));
-   if (matchedEnvironments.length === 0) return null;
-   return { ...s, environments: matchedEnvironments };
- });
+ const matchedResources = await ctx.db
+   .select({
+     systemId: schema.system.id,
+     environmentId: schema.environment.id,
+     resourceId: schema.resource.id
+   })
+   .from(schema.resource)
+   .innerJoin(schema.environment, (join) =>
+     join.where(schema.resourceMatchesMetadata(ctx.db, schema.environment.resourceFilter))
+   )
+   .where(
+     inArray(
+       schema.system.id,
+       systems.map(s => s.id)
+     )
+   );
+ 
+ const matchedSystems = systems
+   .map(s => {
+     const envMatches = matchedResources
+       .filter(m => m.systemId === s.id)
+       .map(m => s.environments.find(e => e.id === m.environmentId))
+       .filter(isPresent);
+     return envMatches.length > 0 ? { ...s, environments: envMatches } : null;
+   })
+   .filter(isPresent);

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

Comment on lines 72 to 78
window.requestAnimationFrame(() => {
// hack to get it to center - we should figure out when the layout is done
// and then call fitView. We are betting that everything should be
// rendered correctly in 100ms before fitting the view.
sleep(100).then(() => fitView({ padding: 0.12, maxZoom: 1 }));
});
}, [getNodes, getEdges, setNodes, setEdges, fitView]);
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 17, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace arbitrary delays with event-driven logic for consistent rendering

Using sleep(100) to delay the fitView call relies on a fixed timeout, which may lead to inconsistent behavior across different devices or datasets. Instead, utilize event-driven mechanisms to trigger fitView after the layout computation is complete, ensuring reliability and better performance.

Consider refactoring your code to call fitView after the nodes and edges have been updated:

       setEdges([...layouted.edges]);

+      // Call fitView after nodes and edges are updated
+      fitView({ padding: 0.12, maxZoom: 1 });
     });
-
-    window.requestAnimationFrame(() => {
-      // hack to get it to center - we should figure out when the layout is done
-      // and then call fitView. We are betting that everything should be
-      // rendered correctly in 100ms before fitting the view.
-      sleep(100).then(() => fitView({ padding: 0.12, maxZoom: 1 }));
-    });

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

Copy link
Member

@jsbroks jsbroks Nov 17, 2024

Choose a reason for hiding this comment

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

@zacharyblasczyk figured a better way to do this. Could you look at how we do this in other reactflows

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Comment on lines 146 to 149
useEffect(() => {
if (reactFlowInstance != null) onLayout();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reactFlowInstance]);
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

Avoid disabling ESLint rules; properly manage dependencies in useEffect

Disabling react-hooks/exhaustive-deps can mask issues with missing dependencies, leading to unintended side effects. Instead, include all necessary dependencies in the dependency array or refactor your code to prevent unnecessary re-renders.

Update the useEffect hook to include onLayout in the dependency array:

 useEffect(() => {
   if (reactFlowInstance != null) onLayout();
-  // eslint-disable-next-line react-hooks/exhaustive-deps
-}, [reactFlowInstance]);
+}, [reactFlowInstance, onLayout]);

Ensure that onLayout is memoized correctly with useCallback and includes all dependencies to prevent re-creation on each render.

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

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

🧹 Outside diff range and nitpick comments (3)
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (3)

36-52: Consider enhancing type safety for edge types

While node types are well-defined with an enum, edge types lack similar type safety. Consider creating an enum for edge types as well.

+enum EdgeType {
+  Default = 'default'
+}

-const edgeTypes: EdgeTypes = { default: DepEdge };
+const edgeTypes: EdgeTypes = { [EdgeType.Default]: DepEdge };

53-67: Consider making layout parameters configurable

The layout parameters (direction "TB", spacing 0, 50) are hardcoded. Consider making these configurable through props for better flexibility.

+type LayoutConfig = {
+  direction?: 'TB' | 'LR' | 'RL' | 'BT';
+  rankSeparation?: number;
+  nodeSeparation?: number;
+};

-const useOnLayout = (setIsLayouted: (isLayouted: boolean) => void) => {
+const useOnLayout = (
+  setIsLayouted: (isLayouted: boolean) => void,
+  config?: LayoutConfig
+) => {
   const { getNodes, setNodes, setEdges, getEdges } = useReactFlow();
   return useCallback(() => {
     const layouted = getLayoutedElementsDagre(
       getNodes(),
       getEdges(),
-      "TB",
-      0,
-      50,
+      config?.direction ?? "TB",
+      config?.rankSeparation ?? 0,
+      config?.nodeSeparation ?? 50,
     );
     setNodes([...layouted.nodes]);
     setEdges([...layouted.edges]);
     setIsLayouted(true);
-  }, [getNodes, getEdges, setNodes, setEdges, setIsLayouted]);
+  }, [getNodes, getEdges, setNodes, setEdges, setIsLayouted, config]);

75-112: Extract node initialization logic

The node initialization logic is complex and could be moved to a separate function for better maintainability.

+const createInitialNodes = (
+  resource: SCHEMA.Resource,
+  systems: Relationships['systems'],
+  provider: Relationships['provider']
+) => compact([
+  {
+    id: resource.id,
+    type: NodeType.Resource,
+    data: { ...resource, label: resource.identifier },
+    position: { x: 0, y: 0 },
+  },
+  ...systems.flatMap((system) =>
+    system.environments.map((env) => ({
+      id: env.id,
+      type: NodeType.Environment,
+      data: { ...env, label: env.name },
+      position: { x: 0, y: 0 },
+    })),
+  ),
+  // ... rest of the node creation logic
+]);

-  const [nodes, _, onNodesChange] = useNodesState<{ label: string }>(
-    compact([
-      {
-        id: resource.id,
-        type: NodeType.Resource,
-        // ... current implementation
-      },
-    ]),
-  );
+  const [nodes, _, onNodesChange] = useNodesState<{ label: string }>(
+    createInitialNodes(resource, systems, provider)
+  );
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ace764b and 56c5edb.

📒 Files selected for processing (4)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/DepEdge.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx (1 hunks)
  • packages/api/src/router/target.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/DepEdge.tsx
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx
  • packages/api/src/router/target.ts
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (3)

1-34: LGTM: Well-structured imports and type definitions

The imports and type definitions are properly organized, with clear separation between external dependencies and local components.


135-138: Previous review comment is still valid: Fix ESLint dependency warning

The ESLint disable comment masks potential issues. The previous review suggestion to properly manage dependencies is still applicable.


169-175: LGTM: Well-implemented provider component

The provider component correctly wraps the main component with ReactFlow's context.

Comment on lines 140 to 150
useEffect(() => {
if (
reactFlowInstance != null &&
nodes.length &&
isLayouted &&
!isViewFitted
) {
reactFlowInstance.fitView({ padding: 0.12, maxZoom: 1 });
setIsViewFitted(true);
}
}, [reactFlowInstance, nodes, isLayouted, isViewFitted]);
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

Improve view fitting logic

The current implementation relies on multiple state flags. Consider using React Flow's built-in onNodesChange callback for more reliable view fitting.

-  useEffect(() => {
-    if (
-      reactFlowInstance != null &&
-      nodes.length &&
-      isLayouted &&
-      !isViewFitted
-    ) {
-      reactFlowInstance.fitView({ padding: 0.12, maxZoom: 1 });
-      setIsViewFitted(true);
-    }
-  }, [reactFlowInstance, nodes, isLayouted, isViewFitted]);

+  const onNodesChange = useCallback((changes) => {
+    _onNodesChange(changes);
+    if (reactFlowInstance && changes.length > 0) {
+      reactFlowInstance.fitView({ padding: 0.12, maxZoom: 1 });
+    }
+  }, [reactFlowInstance]);

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

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 (6)
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (2)

35-51: Consider enhancing type safety for edge types.

The node types configuration is well-structured. For consistency and type safety, consider defining an EdgeType enum similar to NodeType.

+enum EdgeType {
+  Default = "default",
+}

 const nodeTypes: NodeTypes = {
   [NodeType.Resource]: ResourceNode,
   [NodeType.Environment]: EnvironmentNode,
   [NodeType.Provider]: ProviderNode,
   [NodeType.System]: SystemNode,
   [NodeType.Deployment]: DeploymentNode,
 };
-const edgeTypes: EdgeTypes = { default: DepEdge };
+const edgeTypes: EdgeTypes = { [EdgeType.Default]: DepEdge };

56-93: Consider improving initial node positioning strategy.

Currently, all nodes are initialized at position {x: 0, y: 0} before the layout algorithm runs. This could cause visual flickering as nodes move from their initial position to their final layout position.

Consider implementing a basic initial positioning strategy to reduce layout shifts:

+const getInitialPosition = (type: NodeType, index: number) => {
+  const spacing = 200;
+  switch (type) {
+    case NodeType.Resource:
+      return { x: 0, y: 0 };
+    case NodeType.Environment:
+      return { x: spacing, y: index * spacing };
+    case NodeType.System:
+      return { x: spacing * 2, y: index * spacing };
+    case NodeType.Deployment:
+      return { x: spacing * 3, y: index * spacing };
+    case NodeType.Provider:
+      return { x: -spacing, y: 0 };
+  }
+};

Then use it in node creation:

-        position: { x: 0, y: 0 },
+        position: getInitialPosition(NodeType.Environment, index),
packages/api/src/router/resources.ts (1)

339-355: Consider optimizing the query structure for better performance.

The current implementation fetches all systems and environments upfront and then filters them in memory. Consider pushing more filtering to the database level to reduce the data transfer and processing overhead.

 const resource = await ctx.db.query.resource.findFirst({
   where: eq(schema.resource.id, input),
   with: {
     provider: { with: { google: true } },
     workspace: {
       with: {
         systems: {
           with: {
-            environments: { where: hasFilter },
+            environments: {
+              where: and(
+                hasFilter,
+                schema.resourceMatchesMetadata(ctx.db, schema.environment.resourceFilter)
+              )
+            },
             deployments: true,
           },
         },
       },
     },
   },
 });
apps/webservice/src/app/[workspaceSlug]/_components/reactflow/layout.ts (3)

75-118: Consider passing config as a dependency in useCallback

While config is included in the dependency array of onLayout, if any of its properties change, onLayout will recalculate. Ensure that this behavior is intended. If config is expected to remain constant, consider memoizing it or specifying individual config properties in the dependency array for more granular control.


94-95: Optimize state management for reactFlowInstance

Currently, reactFlowInstance is stored in state using useState. Since reactFlowInstance is an object reference that may change frequently, consider using useRef instead to avoid unnecessary re-renders.

Apply this diff to use useRef:

- const [reactFlowInstance, setReactFlowInstance] =
-   useState<ReactFlowInstance | null>(null);
+ const reactFlowInstance = useRef<ReactFlowInstance | null>(null);

And update references accordingly.


117-117: Provide clear usage instructions for useLayoutAndFitView hook

Returning setReactFlowInstance may not be immediately intuitive for users of the hook. Consider returning an object with clearly named properties or providing documentation to guide developers on how to use the hook effectively.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 56c5edb and 80eede6.

📒 Files selected for processing (5)
  • apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (1 hunks)
  • apps/webservice/src/app/[workspaceSlug]/_components/reactflow/layout.ts (2 hunks)
  • apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (3 hunks)
  • packages/api/src/router/resources.ts (2 hunks)
  • packages/db/src/schema/resource.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/db/src/schema/resource.ts
🔇 Additional comments (7)
apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx (3)

1-33: LGTM! Well-structured imports and type definitions.

The imports are properly organized, and the type definitions are clear and properly leverage the schema types.


131-137: LGTM! Clean provider implementation.

The provider component is well-implemented, properly wrapping the main component with ReactFlowProvider and handling prop forwarding correctly.


112-112: Verify the layout hook implementation.

The useLayoutAndFitView hook is crucial for proper diagram rendering. Let's verify its implementation.

✅ Verification successful

The implementation of useLayoutAndFitView has been found and verified. The hook properly handles both layout computation and view fitting through:

  1. Layout management using getLayoutedElementsDagre with configurable parameters
  2. Proper state management for layout and view fitting status
  3. Automatic layout computation when ReactFlow instance is available
  4. View fitting with configurable padding and zoom levels
  5. Correct dependency arrays in the useEffect hooks

The implementation follows React best practices and handles the diagram rendering requirements effectively.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the implementation of useLayoutAndFitView hook
# Expected: The hook should handle layout computation and view fitting

# Search for the hook implementation
ast-grep --pattern 'export const useLayoutAndFitView = $_' -A 20

# Check for any related test files
fd -g "*layout*.test.{ts,tsx}"

Length of output: 4698

apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (1)

5-5: LGTM: Clean implementation of workspace-aware routing

The addition of useParams hook and workspace slug extraction follows Next.js best practices for dynamic routing while maintaining type safety.

Also applies to: 37-37

packages/api/src/router/resources.ts (2)

13-13: LGTM!

The isNotNull import is correctly placed and necessary for the new functionality.


336-337: LGTM!

The procedure signature is well-defined with proper input validation using zod's UUID schema.

apps/webservice/src/app/[workspaceSlug]/_components/reactflow/layout.ts (1)

109-114: Ensure fitView is called only once when conditions are met

The effect depends on several variables, including nodes, isLayouted, and isViewFitted. If any of these change, the effect may re-run. Confirm that setIsViewFitted(true) properly prevents redundant calls to fitView. Additionally, consider whether the conditions accurately reflect when fitView should be invoked.

Comment on lines +128 to +137
<Link
href={`/${workspaceSlug}/targets/${target.id}/visualize`}
className={buttonVariants({
variant: "outline",
size: "sm",
className: "gap-1",
})}
>
<IconTopologyStar3 className="h-3 w-3" /> Visualize
</Link>
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

Visualization route is properly implemented but lacks target type validation

The visualization route and component are properly implemented with:

  • A dedicated page component that fetches resource and relationship data
  • A comprehensive visualization diagram component using ReactFlow
  • Proper error handling with notFound()

However, the implementation doesn't validate target types or states before rendering. Consider:

  • Adding type validation before fetching relationships
  • Conditionally showing the visualization button based on target type/state
  • Adding error boundaries for unsupported target types
🔗 Analysis chain

Verify visualization route and consider conditional rendering

While the implementation looks clean, please ensure:

  1. The visualization route /${workspaceSlug}/targets/${target.id}/visualize is properly implemented
  2. Consider if the visualization button should be conditionally rendered based on target type or state

Let's verify the visualization route implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the visualization page component exists
fd -e tsx -e ts "page" "app/\[workspaceSlug\]/targets/\[targetId\]/visualize"

# Check for any route handlers
fd -e tsx -e ts "route" "app/\[workspaceSlug\]/targets/\[targetId\]/visualize"

# Look for visualization-related components
rg -t tsx -t ts "ResourceVisualizationDiagram|VisualizePage" -l

Length of output: 530


Script:

#!/bin/bash
# Let's try a different approach to find visualization-related files and routes

# Search for visualization-related files in the app directory
fd -t f "visual|visualize" apps/webservice/src/app

# Search for route patterns or components related to visualization
rg -g "*.{ts,tsx}" "visualize|visualization" apps/webservice/src/app

# Look for any page components under targets directory
fd -t f "page.tsx" apps/webservice/src/app/\[workspaceSlug\]/targets

Length of output: 786


Script:

#!/bin/bash
# Let's check the content of the visualization component and look for a page component

# Check the content of ResourceVisualizationDiagram.tsx
cat "apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/ResourceVisualizationDiagram.tsx"

# Look for a page.tsx in the visualize directory
fd -t f "page.tsx" "apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize"

# Check if there are any layout files in the visualize directory
fd -t f "layout.tsx" "apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize"

Length of output: 4435


Script:

#!/bin/bash
# Let's check the content of the visualization page component

# Check the content of the page component
cat "apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize/page.tsx"

# Look for any target type checks or conditional rendering logic
rg -g "*.{ts,tsx}" "type.*Target|targetType" apps/webservice/src/app/[workspaceSlug]/(targets)/targets/[targetId]/visualize

Length of output: 1104

Comment on lines +390 to +392
resource.provider.google.length > 0
? resource.provider.google[0]!
: null,
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

Fix unsafe array access of Google provider.

The current implementation uses the non-null assertion operator (!) which could lead to runtime errors. Consider using optional chaining or proper null checking.

-              google:
-                resource.provider.google.length > 0
-                  ? resource.provider.google[0]!
-                  : null,
+              google: resource.provider.google[0] ?? null,
📝 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
resource.provider.google.length > 0
? resource.provider.google[0]!
: null,
resource.provider.google[0] ?? null,

Comment on lines +97 to +100
useEffect(() => {
if (reactFlowInstance != null) onLayout();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reactFlowInstance]);
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

Avoid disabling react-hooks/exhaustive-deps; ensure dependencies are complete

Disabling the react-hooks/exhaustive-deps ESLint rule can mask potential bugs related to missing dependencies in the useEffect hook. Instead of disabling the rule, include all necessary dependencies to ensure the effect behaves as expected. In this case, onLayout should be added to the dependency array.

Apply this diff to fix the dependency array:

  // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [reactFlowInstance]);
+ }, [reactFlowInstance, onLayout]);
📝 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
useEffect(() => {
if (reactFlowInstance != null) onLayout();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reactFlowInstance]);
useEffect(() => {
if (reactFlowInstance != null) onLayout();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reactFlowInstance, onLayout]);

@adityachoudhari26 adityachoudhari26 merged commit 815959f into main Nov 18, 2024
1 of 9 checks passed
@adityachoudhari26 adityachoudhari26 deleted the resource-relatioships-visualization branch November 18, 2024 03:38
@coderabbitai coderabbitai bot mentioned this pull request Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants