Clients/src/presentation/components/AddNewRiskForm/index.tsx (2)

524-554: Well-structured field mapping with clear prefixes.

The prefixing approach effectively distinguishes risk vs mitigation fields and the mapping is comprehensive.

Consider extracting the field mapping into a typed constant at the module level for better maintainability and type safety:

const FIELD_MAPPING = {
  risk: {
    riskName: 'risk_name',
    actionOwner: 'risk_owner',
    // ... other risk fields
  },
  mitigation: {
    mitigationStatus: 'mitigation_status',
    // ... other mitigation fields
  }
} as const;

---

634-664: Consider stronger typing for changedFields object.

The logic for computing changed fields separately and combining with prefixes is sound, and the boolean flags correctly use prefixed keys.

Based on learnings, avoid using any type when a more specific type is available. Consider:

-        const changedFields: any = {};
+        const changedFields: Record<string, any> = {};

Or better yet, create a proper type:

type PrefixedRiskFields = { [K in keyof RiskFormValues as `risk_${string & K}`]: RiskFormValues[K] };
type PrefixedMitigationFields = { [K in keyof MitigationFormValues as `mitigation_${string & K}`]: MitigationFormValues[K] };
type ChangedFields = Partial<PrefixedRiskFields & PrefixedMitigationFields>;

const changedFields: ChangedFields = {};

Clients/src/presentation/pages/RiskManagement/index.tsx (1)

390-405: Use theme references instead of hardcoded color values.

The Analytics button uses hardcoded color values (#7F56D9, #6941C6) which should be replaced with theme references for consistency and maintainability.

Apply this diff to use theme colors:

             <div data-joyride-id="analytics-button">
               <CustomizableButton
                 variant="contained"
                 text="Analytics"
                 sx={{
-                  backgroundColor: "#7F56D9",
-                  border: "1px solid #7F56D9",
+                  backgroundColor: "primary.main",
+                  border: "1px solid",
+                  borderColor: "primary.main",
                   gap: 2,
                   "&:hover": {
-                    backgroundColor: "#6941C6",
+                    backgroundColor: "primary.dark",
                   },
                 }}
                 onClick={() => setIsAnalyticsDrawerOpen(true)}
                 icon={<TrendingUp size={16} />}
               />
             </div>

Servers/scripts/createNewTenant.ts (1)

977-993: Consider adding indexes for query performance.

The history tables are properly structured with appropriate constraints. However, for better query performance on time-series data retrieval, consider adding indexes on frequently queried columns.

Add indexes after table creation:

await sequelize.query(`CREATE TABLE "${tenantHash}".risk_history (
  id SERIAL PRIMARY KEY,
  parameter VARCHAR(255) NOT NULL,
  snapshot_data JSONB NOT NULL DEFAULT '{}',
  recorded_at TIMESTAMP NOT NULL DEFAULT NOW(),
  triggered_by_user_id INTEGER REFERENCES public.users(id) ON DELETE SET NULL,
  created_at TIMESTAMP DEFAULT NOW()
);`, { transaction });

// Add indexes for performance
await sequelize.query(`
  CREATE INDEX idx_risk_history_parameter ON "${tenantHash}".risk_history(parameter);
  CREATE INDEX idx_risk_history_recorded_at ON "${tenantHash}".risk_history(recorded_at DESC);
`, { transaction });

await sequelize.query(`CREATE TABLE "${tenantHash}".model_inventory_history (
  id SERIAL PRIMARY KEY,
  parameter VARCHAR(255) NOT NULL,
  snapshot_data JSONB NOT NULL DEFAULT '{}',
  recorded_at TIMESTAMP NOT NULL DEFAULT NOW(),
  triggered_by_user_id INTEGER REFERENCES public.users(id) ON DELETE SET NULL,
  created_at TIMESTAMP DEFAULT NOW()
);`, { transaction });

// Add indexes for performance
await sequelize.query(`
  CREATE INDEX idx_model_inventory_history_parameter ON "${tenantHash}".model_inventory_history(parameter);
  CREATE INDEX idx_model_inventory_history_recorded_at ON "${tenantHash}".model_inventory_history(recorded_at DESC);
`, { transaction });

Clients/src/presentation/pages/ModelInventory/index.tsx (2)

877-896: Use theme references for button styling.

The Analytics button reuses addNewModelButtonStyle which contains hardcoded color values (#13715B). Consider creating a theme-based style or using theme references directly.

Replace the hardcoded style:

               <CustomizableButton
                 variant="contained"
                 onClick={() => setIsAnalyticsDrawerOpen(true)}
-                sx={addNewModelButtonStyle}
+                sx={{
+                  backgroundColor: "secondary.main",
+                  border: "1px solid",
+                  borderColor: "secondary.main",
+                }}
                 icon={<TrendingUp size={16} />}
                 text="Analytics"
               />

---

1009-1021: Consider adding more analytics parameters for comprehensive insights.

The analytics drawer currently only tracks "status" parameter. Consider adding additional parameters like provider, security_assessment status, or other relevant model inventory attributes to provide more comprehensive analytics.

Example:

       <AnalyticsDrawer
         open={isAnalyticsDrawerOpen}
         onClose={() => setIsAnalyticsDrawerOpen(false)}
         title="Analytics & Trends"
         description="Track your model inventory history over time"
         entityName="Model"
+        chartType="modelInventory"
         availableParameters={[
           { value: "status", label: "Status" },
-          // Add more parameters here as needed
+          { value: "provider", label: "Provider" },
+          { value: "security_assessment", label: "Security Assessment" },
         ]}
         defaultParameter="status"
       />

Servers/utils/modelInventory.utils.ts (1)

255-262: Consider tracking additional parameters.

Currently only the status parameter is tracked for model inventory history. Based on the interface comments in i.modelInventoryHistory.ts that mention security_assessment as an example, consider whether other parameters like security_assessment should also be tracked.

Servers/database/migrations/20251105000000-create-model-inventory-history-table.js (1)

27-43: Review index strategy for potential redundancy.

Three indexes are created:

1. Single column on parameter
2. Single column on recorded_at DESC
3. Composite on (parameter, recorded_at DESC)

The composite index can serve queries filtering by parameter alone, making the single-column parameter index potentially redundant. Consider whether all three indexes are needed based on actual query patterns, as each index adds overhead to write operations.

Clients/src/presentation/components/Charts/ModelInventoryHistoryChart.tsx (1)

41-63: Satisfy the hooks dependency rule

fetchTimeseriesData is defined outside the effect but isn’t part of the dependency array, which will trip react-hooks/exhaustive-deps in our lint pipeline. To keep the build green, either wrap the fetcher in useCallback and include it in the dependency list or inline the async logic inside the effect.

-  useEffect(() => {
-    fetchTimeseriesData();
-  }, [timeframe, parameter]);
-
-  const fetchTimeseriesData = async () => {
+  useEffect(() => {
+    const fetchTimeseriesData = async () => {
       setLoading(true);
       setError(null);
       try {
         const response = await getModelInventoryTimeseries(parameter, timeframe);
         if (response?.data?.data) {
           setTimeseriesData(response.data.data.data);
         }
       } catch (err: any) {
         console.error("Error fetching timeseries data:", err);
         setError(err?.response?.data?.message || "Failed to load chart data");
       } finally {
         setLoading(false);
       }
     };
-  };
+
+    fetchTimeseriesData();
+  }, [parameter, timeframe]);

Servers/utils/history/riskHistory.utils.ts (1)

50-182: Consider extracting common counting pattern to reduce duplication.

The function contains four nearly identical blocks (severity, likelihood, mitigation_status, risk_level) that follow the same pattern: query counts, initialize all possible values to 0, then update with actual counts. This duplication increases maintenance burden.

Consider extracting a helper function:

async function getCountsForEnum(
  column: string,
  possibleValues: string[],
  tenant: string,
  transaction?: Transaction
): Promise<Record<string, number>> {
  const counts: Record<string, number> = {};
  
  const results = await sequelize.query(
    `SELECT ${column}, COUNT(*) as count
     FROM "${tenant}".risks
     WHERE is_deleted = false
     GROUP BY ${column}`,
    { type: QueryTypes.SELECT, transaction }
  ) as Array<{ [key: string]: any; count: string }>;
  
  possibleValues.forEach(value => counts[value] = 0);
  results.forEach(row => {
    if (row[column]) counts[row[column]] = parseInt(row.count, 10);
  });
  
  return counts;
}

Then simplify the main function to use this helper for severity, likelihood, mitigation_status, and risk_level.

Servers/utils/history/modelInventoryHistory.utils.ts (1)

1-481: Consider abstracting shared history logic to eliminate duplication.

This file is nearly identical to riskHistory.utils.ts (Servers/utils/history/riskHistory.utils.ts), with only table names and parameter-specific logic differing. This duplication means changes must be maintained in two places.

Consider creating a generic history utility that both files can use:

// historyBase.utils.ts
export function createHistoryUtils<TModel>(config: {
  tableName: string;
  historyTableName: string;
  model: typeof Model;
  parameterHandlers: Record<string, ParameterHandler>;
}) {
  return {
    recordHistorySnapshot: async (parameter, tenant, userId?, tx?) => {
      // Generic implementation using config.historyTableName
    },
    getCurrentParameterCounts: async (parameter, tenant, tx?) => {
      const handler = config.parameterHandlers[parameter];
      if (handler) return handler(tenant, tx);
      // Generic fallback with validation
    },
    // ... other functions
  };
}

Then instantiate for each domain:

// riskHistory.utils.ts
export const riskHistoryUtils = createHistoryUtils({
  tableName: 'risks',
  historyTableName: 'risk_history',
  model: RiskHistoryModel,
  parameterHandlers: { severity: handleSeverity, ... }
});

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: MuhammadKhalilzadeh
Repo: bluewave-labs/verifywise PR: 254
File: Clients/src/presentation/components/Modals/Controlpane/index.tsx:22-24
Timestamp: 2024-11-18T05:28:01.115Z
Learning: In `Clients/src/presentation/components/Modals/Controlpane/index.tsx` (TypeScript React component), avoid changing prop types from specific types like `string` to `any` when the exact type is known. Maintaining explicit types ensures better type safety and code reliability.

Clients/src/presentation/components/AddNewRiskForm/index.tsx (2)

431-454: Avoid any in generic types for better type safety.

The generic constraint Record<string, any> and return type Record<string, any> reduce type safety. Consider using more specific types or at least unknown instead of any. Based on learnings.

Additionally, the array comparison at lines 441-443 only works correctly for arrays of primitives or simple objects. For nested objects, consider using a deep equality library or a more robust comparison method.

Apply this diff to improve type safety:

-  const getChangedFields = useCallback(<T extends Record<string, any>>(original: Partial<T>, current: Partial<T>) => {
-    const changedFields: Record<string, any> = {};
+  const getChangedFields = useCallback(<T extends Record<string, unknown>>(original: Partial<T>, current: Partial<T>): Partial<T> => {
+    const changedFields: Partial<T> = {};

For the array comparison, consider adding a comment clarifying the limitation:

-      // For arrays, do deep comparison (use copies to avoid mutating originals)
+      // For arrays of primitives, do sorted comparison (use copies to avoid mutating originals)
+      // Note: This comparison may not work correctly for arrays of complex nested objects
       if (Array.isArray(originalValue) && Array.isArray(currentValue)) {

---

524-553: Consider type-safe field mapping.

The manual field mapping is extensive and could be error-prone during maintenance. Consider using TypeScript's type system to ensure the mapping keys match the actual field names, or extract this mapping to a shared constant file for reusability.

For example, you could create a type-safe mapping helper:

// Define types for risk and mitigation field names
type RiskFieldKey = keyof RiskFormValues;
type MitigationFieldKey = keyof MitigationFormValues;

// Create a mapping that ensures compile-time checking
const createFieldMapping = () => {
  const riskMapping: Record<`risk_${RiskFieldKey}`, string> = {
    risk_riskName: 'risk_name',
    risk_actionOwner: 'risk_owner',
    // ... other risk fields
  };
  
  const mitigationMapping: Record<`mitigation_${MitigationFieldKey}`, string> = {
    mitigation_mitigationStatus: 'mitigation_status',
    // ... other mitigation fields
  };
  
  return { ...riskMapping, ...mitigationMapping };
};

Clients/src/presentation/components/Charts/ModelInventoryHistoryChart.tsx (3)

39-39: Define a proper type interface for timeseries data.

The any[] type bypasses TypeScript's type checking and could hide bugs. Define an interface that matches the expected shape of the API response.

Consider creating an interface like:

interface TimeseriesDataPoint {
  timestamp: string;
  data: Record<ModelInventoryStatus, number>;
}

Then update the state declaration:

-  const [timeseriesData, setTimeseriesData] = useState<any[]>([]);
+  const [timeseriesData, setTimeseriesData] = useState<TimeseriesDataPoint[]>([]);

---

66-92: Memoize expensive chart data preparation.

prepareChartData recalculates on every render, even when timeseriesData hasn't changed. This is inefficient, especially with large datasets.

Apply this diff:

+  const { timestamps, series, maxValue } = React.useMemo(() => {
-  // Prepare data for the chart
-  const prepareChartData = () => {
     if (!timeseriesData || timeseriesData.length === 0) {
       return { timestamps: [], series: [], maxValue: 0 };
     }
 
     const timestamps = timeseriesData.map((point) => new Date(point.timestamp));
 
     // Get all unique status values
     const statusValues = Object.values(ModelInventoryStatus);
     const series = statusValues.map((status) => ({
       label: status,
       data: timeseriesData.map((point) => point.data[status] || 0),
       color: STATUS_COLORS[status] || "#9CA3AF",
       curve: "monotoneX" as const,
       showMark: false,
     }));
 
     // Calculate max value across all series for proper y-axis scaling
     const maxValue = Math.max(
       ...series.flatMap((s) => s.data),
       0
     );
 
     return { timestamps, series, maxValue };
-  };
-
-  const { timestamps, series, maxValue } = prepareChartData();
+  }, [timeseriesData]);

---

94-110: Extract magic number to a named constant.

The 120 added to height appears in multiple places (lines 101, 119, 139) but its purpose isn't clear. This represents the additional space needed for title, controls, and padding.

Consider extracting to a constant at the top of the file:

+const CHART_CONTAINER_EXTRA_HEIGHT = 120; // Space for title, controls, and padding
+
 const ModelInventoryHistoryChart: React.FC<ModelInventoryHistoryChartProps> = ({

Then use it consistently:

-          height: height + 120,
+          height: height + CHART_CONTAINER_EXTRA_HEIGHT,

Clients/src/presentation/components/Charts/RiskHistoryChart.tsx (4)

8-12: Consider exporting the interface for consumers.

The RiskHistoryChartProps interface is not currently exported. If other components need to reference this type (e.g., when wrapping or extending this component), they won't have access to it.

Apply this diff to export the interface:

-interface RiskHistoryChartProps {
+export interface RiskHistoryChartProps {
   parameter?: string;
   title?: string;
   height?: number;
 }

---

83-83: Replace any[] with a typed interface.

Using any[] for timeseriesData bypasses TypeScript's type checking and can lead to runtime errors if the data structure changes.

Define an interface for the timeseries data:

interface TimeseriesPoint {
  timestamp: string;
  data: Record<string, number>;
}

Then update the state declaration:

-  const [timeseriesData, setTimeseriesData] = useState<any[]>([]);
+  const [timeseriesData, setTimeseriesData] = useState<TimeseriesPoint[]>([]);

---

85-103: Consider wrapping fetchTimeseriesData in useCallback.

The function fetchTimeseriesData is called in the useEffect but isn't listed in the dependency array, which may trigger ESLint warnings. While the current implementation is functionally correct (the function captures parameter and timeframe from its closure), wrapping it in useCallback would make the dependency flow explicit and satisfy the linter.

-  const fetchTimeseriesData = async () => {
+  const fetchTimeseriesData = useCallback(async () => {
     setLoading(true);
     setError(null);
     try {
       const response = await getRiskTimeseries(parameter, timeframe);
       if (response?.data?.data) {
         setTimeseriesData(response.data.data.data);
       }
     } catch (err: any) {
       console.error("Error fetching timeseries data:", err);
       setError(err?.response?.data?.message || "Failed to load chart data");
     } finally {
       setLoading(false);
     }
-  };
+  }, [parameter, timeframe]);
 
   useEffect(() => {
     fetchTimeseriesData();
-  }, [timeframe, parameter]);
+  }, [fetchTimeseriesData]);

---

247-247: Extract magic numbers as named constants.

The values 1.15 (y-axis padding multiplier) and 10 (fallback maximum) are magic numbers that reduce code readability. Extracting them as named constants would make their purpose clearer.

Add constants near the top of the file (after the color maps):

const Y_AXIS_PADDING_MULTIPLIER = 1.15; // Add 15% padding above max value
const Y_AXIS_DEFAULT_MAX = 10; // Default max when no data

Then update line 247:

-                max: maxValue > 0 ? Math.ceil(maxValue * 1.15) : 10,
+                max: maxValue > 0 ? Math.ceil(maxValue * Y_AXIS_PADDING_MULTIPLIER) : Y_AXIS_DEFAULT_MAX,

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: MuhammadKhalilzadeh
Repo: bluewave-labs/verifywise PR: 254
File: Clients/src/presentation/components/Modals/Controlpane/index.tsx:22-24
Timestamp: 2024-11-18T05:28:01.115Z
Learning: In `Clients/src/presentation/components/Modals/Controlpane/index.tsx` (TypeScript React component), avoid changing prop types from specific types like `string` to `any` when the exact type is known. Maintaining explicit types ensures better type safety and code reliability.