packages/api/src/sequelize/migrations/2025-04-28_02_create-patient_import_mapping-table.ts (2)

11-13: Comment should be more descriptive.

The comment "Use 'Promise.all' when changes are independent of each other" doesn't match what the code is actually doing. This migration is using sequential operations within a transaction, not parallel Promise.all execution.

-// Use 'Promise.all' when changes are independent of each other
+// Create patient import mapping table to associate patient records with bulk import jobs

---

51-57: Use dynamic constraint name from PostgreSQL.

Rather than hardcoding the constraint name, let PostgreSQL dynamically generate it to avoid potential naming conflicts when the migration is applied to different environments.

-    await queryInterface.addConstraint(tableName, {
-      name: constraintName,
-      fields: constraintFieldNames,
-      type: "unique",
-      transaction,
-    });
+    await queryInterface.addConstraint(tableName, {
+      fields: constraintFieldNames,
+      type: "unique",
+      transaction,
+    });

packages/api/src/sequelize/migrations/2025-04-28_01_alter-patient_import-add-change-log.ts (3)

15-16: Comment doesn't match implementation.

The comment suggests using Promise.all for independent changes, but the implementation doesn't use Promise.all since there's only a single operation.

-// Use 'Promise.all' when changes are independent of each other
+// Add change logging trigger to patient_import table

---

16-20: Consider validating trigger existence before creation.

The migration could fail if the trigger already exists. Consider adding a condition to check if the trigger already exists before trying to create it.

export const up: Migration = async ({ context: queryInterface }) => {
  await queryInterface.sequelize.transaction(async transaction => {
+    // Check if trigger exists first
+    const [results] = await queryInterface.sequelize.query(
+      `SELECT 1 FROM pg_trigger WHERE tgname = '${triggerName}'`,
+      { transaction }
+    );
+    if ((results as any[]).length === 0) {
      await queryInterface.sequelize.query(getChangeTriggerCreateCmd(), { transaction });
+    }
  });
};

---

22-26: Consider using a transaction-aware query for down migration.

For consistency with the approach in up, consider using the raw query approach with sequelize.query instead of the dropTrigger method, as this ensures consistent transaction handling.

export const down: Migration = ({ context: queryInterface }) => {
  return queryInterface.sequelize.transaction(async transaction => {
-    await queryInterface.dropTrigger(tableName, triggerName, { transaction });
+    await queryInterface.sequelize.query(
+      `DROP TRIGGER IF EXISTS ${triggerName} ON ${tableName}`,
+      { transaction }
+    );
  });
};

packages/api/src/sequelize/migrations/2025-04-28_00_create-patient_import-table.ts (5)

7-8: Comment doesn't match implementation.

The comment suggests using Promise.all for independent changes, but the implementation doesn't use Promise.all since there's only a single createTable operation.

-// Use 'Promise.all' when changes are independent of each other
+// Create patient_import table to store bulk import job status and metadata

---

33-36: Consider adding length limit to reason field.

The reason field is defined as a string without length constraints. Consider adding a maximum length to prevent excessively large values.

reason: {
  type: DataTypes.STRING,
+  // Specify reasonable length limit for error messages
+  type: DataTypes.STRING(1024),
  allowNull: true,
},

---

47-61: Add comments explaining counter fields.

The counter fields (total, successful, failed) would benefit from brief comments explaining their purpose in the context of patient imports.

total: {
  type: DataTypes.INTEGER,
  allowNull: false,
  defaultValue: 0,
+  // Total number of patient records in the import job
},
successful: {
  type: DataTypes.INTEGER,
  allowNull: false,
  defaultValue: 0,
+  // Count of successfully imported patient records
},
failed: {
  type: DataTypes.INTEGER,
  allowNull: false,
  defaultValue: 0,
+  // Count of failed patient record imports
},

---

62-71: Consider adding JSONB validation or schema documentation.

The JSONB fields paramsCx and paramsOps don't have any documentation about their expected structure. Consider adding comments documenting the expected schema.

paramsCx: {
  type: DataTypes.JSONB,
  field: "params_cx",
  allowNull: false,
+  // Customer-specific parameters for the import job
+  // Expected schema: { ... }
},
paramsOps: {
  type: DataTypes.JSONB,
  field: "params_ops",
  allowNull: false,
+  // Operational parameters for the import job
+  // Expected schema: { ... }
},

---

13-72: Consider adding indexes for frequent query patterns.

The table might benefit from additional indexes on frequently queried columns like cx_id, facility_id, or status to improve query performance, especially for listing/filtering operations.

// After creating the table
await shared.createTable(
  // ... existing code
);
+// Add indexes for frequently queried columns
+await queryInterface.addIndex(jobTableName, {
+  fields: ["cx_id"],
+  transaction,
+});
+await queryInterface.addIndex(jobTableName, {
+  fields: ["facility_id"],
+  transaction,
+});
+await queryInterface.addIndex(jobTableName, {
+  fields: ["status"],
+  transaction,
+});

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite

Learnt from: leite08
PR: metriport/metriport#3412
File: packages/api/src/models/medical/patient-import-mapping.ts:16-41
Timestamp: 2025-04-26T22:17:45.228Z
Learning: The patient_import_mapping table already has a unique constraint on the fields "job_id", "row_number", and "data_pipeline_request_id" defined in the migration file 2025-04-24_02_create-patient_import_mapping-table.ts, which ensures data integrity.