packages/shared/src/medical/fhir/resources.ts (1)

84-88: Consider refining the type predicate implementation.

The type predicate function implementation casts the input to the target type before checking if it exists in the array. This approach contradicts the coding guidelines which state that type predicates should "only [be] applicable to narrow down the type, not to force a complete type conversion."

A more appropriate implementation would be:

export function isResourceTypeForConsolidation(
  resourceType: string
): resourceType is ResourceTypeForConsolidation {
-  return resourceTypeForConsolidation.includes(resourceType as ResourceTypeForConsolidation);
+  return resourceTypeForConsolidation.some(type => type === resourceType);
}

This implementation avoids the type casting and performs a proper runtime check that aligns better with the purpose of type predicates.

packages/shared/src/interface/external/ehr/resource-diff.ts (1)

5-7: Type guard implementation is correct but could be improved

The type guard correctly checks if a string is a valid ResourceDiffDirection value, but the type assertion can be avoided.

export function isResourceDiffDirection(direction: string): direction is ResourceDiffDirection {
-  return Object.values(ResourceDiffDirection).includes(direction as ResourceDiffDirection);
+  return Object.values(ResourceDiffDirection).includes(direction as any);
}

Alternatively, for even better type safety:

export function isResourceDiffDirection(direction: string): direction is ResourceDiffDirection {
-  return Object.values(ResourceDiffDirection).includes(direction as ResourceDiffDirection);
+  return Object.values(ResourceDiffDirection).some(value => value === direction);
}

packages/core/src/external/ehr/resource-diff/compute/ehr-compute-resource-diff-factory.ts (1)

6-13: Good use of factory pattern for environment-specific implementation.

The factory function correctly determines which implementation to return based on the environment, following the coding guidelines for keeping functions small and deterministic.

Consider adding a brief comment explaining why a waitTime of 0 is used for the local implementation.

 export function buildEhrComputeResourceDiffHandler(): EhrComputeResourceDiffHandler {
   if (Config.isDev()) {
+    // Local implementation with no delay for development environment
     const waitTimeAtTheEndInMillis = 0;
     return new EhrComputeResourceDiffLocal(waitTimeAtTheEndInMillis);
   }
   const ehrComputeResourceDiffQueueUrl = Config.getEhrComputeResourceDiffQueueurl("");
   return new EhrComputeResourceDiffCloud(ehrComputeResourceDiffQueueUrl);
 }

packages/core/src/external/ehr/api/save-resource-diff.ts (2)

35-41: Validate matchedResourceIds array before joining.

The code joins the matchedResourceIds array with commas without validation. Consider adding validation to ensure the array contains valid IDs before joining.

-  const queryParams = new URLSearchParams({
-    cxId,
-    patientId,
-    resourceId,
-    direction,
-    matchedResourceIds: matchedResourceIds.join(","),
-  });
+  const queryParams = new URLSearchParams({
+    cxId,
+    patientId,
+    resourceId,
+    direction,
+    matchedResourceIds: matchedResourceIds.filter(Boolean).join(","),
+  });

---

43-47: Consider returning the response data.

The function only verifies that response data exists but doesn't return it. If the caller never needs the response data, this is fine, but if they might need it in the future, consider modifying the return type and returning the response data.

-export async function saveResourceDiff({
+export async function saveResourceDiff<T = any>({
   ehr,
   cxId,
   patientId,
   resourceId,
   direction,
   matchedResourceIds,
 }: {
   ehr: EhrSource;
   cxId: string;
   patientId: string;
   resourceId: string;
   direction: ResourceDiffDirection;
   matchedResourceIds: string[];
-}): Promise<void> {
+}): Promise<T> {
   const { log, debug } = out(`Ehr saveResourceDiff - cxId ${cxId}`);
   const api = axios.create({ baseURL: Config.getApiurl("") });
   const queryParams = new URLSearchParams({
     cxId,
     patientId,
     resourceId,
     direction,
     matchedResourceIds: matchedResourceIds.join(","),
   });
   const saveResourceDiffUrl = `/internal/ehr/${ehr}/patient/save-resource-diff?${queryParams.toString()}`;
   try {
     const response = await api.post(saveResourceDiffUrl);
     if (!response.data) throw new Error(`No body returned from ${saveResourceDiffUrl}`);
     debug(`${saveResourceDiffUrl} resp: ${JSON.stringify(response.data)}`);
+    return response.data;
   } catch (error) {

packages/infra/lib/api-stack/api-service.ts (1)

80-156: Consider refactoring large function.

This function is over 500 lines long and handles many different concerns. Consider breaking it down into smaller, more focused functions for better maintainability.

For example, you could extract queue setup, environment variable setup, and access permissions into separate helper functions.

packages/api/src/external/ehr/canvas/command/compute-resource-diff.ts (1)

68-68: Consider returning computation results.

The function returns void, which means callers can't know what differences were found. Consider returning the result of the computation.

If ehrResourceDiffHandler.computeResourceDiff() returns useful information, modify this function to return that data to the caller.

packages/api/src/models/resource-mapping-reversed.ts (1)

20-48: Add database constraints and indexes for better performance.

The model definition doesn't include any unique constraints or indexes, which could impact database integrity and query performance.

 static setup: ModelSetup = (sequelize: Sequelize) => {
   ResourceMappingReversedModel.init(
     {
       ...BaseModel.attributes(),
       cxId: {
         type: DataTypes.UUID,
+        allowNull: false,
       },
       patientId: {
         type: DataTypes.UUID,
+        allowNull: false,
       },
       patientMappingExternalId: {
         type: DataTypes.STRING,
+        allowNull: false,
       },
       resourceId: {
         type: DataTypes.STRING,
+        allowNull: false,
       },
       source: {
         type: DataTypes.STRING,
+        allowNull: false,
       },
       externalId: {
         type: DataTypes.STRING,
+        allowNull: false,
       },
     },
     {
       ...BaseModel.modelOptions(sequelize),
       tableName: ResourceMappingReversedModel.NAME,
+      indexes: [
+        {
+          unique: true,
+          fields: ["patient_id", "resource_id", "source"],
+          name: "resource_mapping_reversed_unique_idx",
+        },
+        {
+          fields: ["patient_mapping_external_id"],
+          name: "resource_mapping_reversed_patient_mapping_idx",
+        },
+      ],
     }
   );
 };

packages/core/src/external/ehr/api/compute-resource-diff.ts (1)

9-18: JSDoc parameters don't match function signature

The JSDoc comment includes parameters resourceId and matchedResourceIds which are not present in the actual function signature. Instead, the function accepts a resource parameter which isn't documented.

Update the JSDoc to match the actual function parameters:

/**
 * Sends a request to the API to sync a patient with Metriport.
 *
 * @param ehr - The EHR source.
 * @param cxId - The CX ID.
 * @param patientId - The patient ID.
- * @param resourceId - The resource ID.
+ * @param practiceId - The practice ID.
+ * @param resource - The FHIR resource.
 * @param direction - The direction of the resource diff.
- * @param matchedResourceIds - The matched resource IDs.
 */

packages/api/src/sequelize/migrations/2025-04-03_00_create-resource-mapping-reversed.ts (1)

16-17: Comment is misleading for sequential operations

The comment suggests using Promise.all for independent operations, but the migration operations are inherently sequential (constraints depend on table creation), so awaiting them one after another is correct.

Either remove this comment or clarify that in this specific case, sequential operations are required:

-// Use 'Promise.all' when changes are independent of each other
+// Operations must be sequential as later operations depend on the table being created first

packages/api/src/routes/ehr/canvas/patient.ts (3)

63-69: Enhance API documentation with more details

The JSDoc comment could be more descriptive about what the resource diff process does, the purpose of the direction parameter, and what clients should expect.

/**
 * POST /ehr/canvas/patient/:id/resource-diff
 *
- * Starts the resource diff process
+ * Starts the resource diff process to identify differences between resources in the 
+ * specified direction (EHR to Metriport or Metriport to EHR)
 * @param req.params.id The ID of Canvas Patient.
+ * @param req.query.practiceId The ID of the Canvas Practice.
+ * @param req.query.direction The direction of the diff (DIFF_EHR or DIFF_METRIPORT).
 * @returns 200 OK
 */

---

87-93: Enhance API documentation with response details

The JSDoc comment should include information about the response structure to improve API usability.

/**
 * GET /ehr/canvas/patient/:id/resource-diff
 *
- * Retrieves the resource diff
+ * Retrieves the resource differences between Canvas and Metriport
 * @param req.params.id The ID of Canvas Patient.
+ * @param req.query.direction The direction of the diff (DIFF_EHR or DIFF_METRIPORT).
- * @returns Resource diff
+ * @returns Array of ResourceMappingReversedMapped objects containing resource IDs and mapping information
 */

---

94-108: Consider adding query parameter validation for robustness

While the direction parameter is validated, there's no validation for the existence of other required parameters (unlike in the POST route where you validate practiceId).

router.get(
  "/:id/resource-diff",
  handleParams,
  requestLogger,
  asyncHandler(async (req: Request, res: Response) => {
    const cxId = getCxIdOrFail(req);
    const canvasPatientId = getFrom("params").orFail("id", req);
    const direction = getFromQueryOrFail("direction", req);
    if (!isResourceDiffDirection(direction)) {
      throw new BadRequestError("Invalid direction", undefined, { direction });
    }
+    // Optional: If practiceId is needed for context in getCanvasResourceDiffFromEhr
+    // const canvasPracticeId = getFromQueryOrFail("practiceId", req);
    const resourceIds = await getCanvasResourceDiffFromEhr({ cxId, canvasPatientId, direction });
    return res.status(httpStatus.OK).json(resourceIds);
  })
);

packages/core/src/external/ehr/resource-diff/start/ehr-start-resource-diff-cloud.ts (1)

14-19: Consider simplifying the SQS client initialization logic.
You could unify the assignment into a single statement for brevity.

-    if (!sqsClient) {
-      this.sqsClient = new SQSClient({ region: region ?? Config.getAWSRegion() });
-    } else {
-      this.sqsClient = sqsClient;
-    }
+    this.sqsClient = sqsClient ?? new SQSClient({ region: region ?? Config.getAWSRegion() });

packages/lambdas/src/ehr-process-resource-diff.ts (2)

23-43: Consider processing all SQS messages in a single invocation.
Currently, only the first message in the event is handled. If multiple messages arrive, reviewing them all might be needed.

-  const message = getSingleMessageOrFail(event.Records, lambdaName);
-  if (!message) return;
-  // process the single message
+  for (const record of event.Records) {
+    if (!record.body) continue;
+    const parsedBody = parseBody(record.body);
+    // process each message
+  }

---

64-73: Handle JSON.parse errors gracefully.
Catching parsing errors can help generate a more direct failure message if the payload isn't valid JSON.

-function parseBody(body?: unknown): ProcessResourceDiffRequest {
-  if (!body) throw new MetriportError(`Missing message body`);
-  const bodyString = typeof body === "string" ? (body as string) : undefined;
-  if (!bodyString) throw new MetriportError(`Invalid body`);
-  const bodyAsJson = JSON.parse(bodyString);
-  return ehrResourceDiffSchema.parse(bodyAsJson) as ProcessResourceDiffRequest;
+function parseBody(body?: unknown): ProcessResourceDiffRequest {
+  if (!body) throw new MetriportError("Missing message body");
+  const bodyString = typeof body === "string" ? body : undefined;
+  if (!bodyString) throw new MetriportError("Invalid body");
+  try {
+    const bodyAsJson = JSON.parse(bodyString);
+    return ehrResourceDiffSchema.parse(bodyAsJson) as ProcessResourceDiffRequest;
+  } catch (error) {
+    throw new MetriportError("Failed to parse message body as JSON", error);
+  }
}

packages/core/src/external/ehr/resource-diff/compute/ehr-compute-resource-diff-cloud.ts (1)

11-12: Constant definition is appropriately named.

MAX_SQS_MESSAGE_SIZE is descriptive. Consider moving it to a central constants file if this limit might be reused.

packages/api/src/routes/internal/ehr/canvas/patient.ts (2)

64-92: New /compute-resource-diff endpoint logic is well-structured.

1. Validates direction via isResourceDiffDirection before proceeding.
2. Uses fhirResourceSchema for request body validation, conforming to typed FHIR.
3. Non-blocking computeCanvasResourceDiff call is wrapped in a catch, ensuring asynchronous errors are handled by processAsyncError.

Consider returning a JSON payload for more transparency (e.g., a short success message) instead of an empty 200.

---

94-122: New /save-resource-diff endpoint structure follows a similar pattern.

1. Direction validation is consistent with the prior endpoint.
2. The function saveCanvasResourceDiff is called asynchronously with error handling.
3. Efficient parameter parsing from query is good; ensure callers always pass arrays for matchedResourceIds.

Same comment applies about possibly returning a JSON message or object confirming success.

packages/core/src/external/ehr/resource-diff/utils.ts (1)

1-19: Imports are extensive yet domain-focused.

They rely on multiple deduplication functions, indicating a modular approach. Keep evaluating if all resource-specific modules are truly needed to maintain minimal overhead.

packages/api/src/external/ehr/canvas/command/save-resource-diff.ts (1)

16-43: Consider adding validation for edge cases.

The function correctly handles the DIFF_EHR direction and provides an informative error for the DIFF_METRIPORT direction. However, there are a few considerations:

1. There's no handling for a case where direction doesn't match either of the defined enum values.
2. The matchedResourceIds.sort().join(",") approach assumes IDs won't contain commas, which could lead to parsing ambiguity.

For the sorting and joining of IDs, consider:

- externalId: matchedResourceIds.sort().join(","),
+ externalId: matchedResourceIds.sort().join("|"), // Or another delimiter less likely to appear in IDs

For handling unexpected directions:

  } else if (direction === ResourceDiffDirection.DIFF_METRIPORT) {
    throw new MetriportError("Cannot save resources in this direction", undefined, { direction });
+ } else {
+   throw new MetriportError("Invalid resource diff direction", undefined, { direction });
  }

packages/core/src/external/ehr/canvas/index.ts (1)

352-393: Robust implementation of FHIR resource retrieval.

The new method follows functional programming principles with good error handling, particularly for the NotFoundError case. A few recommendations:

1. Consider strengthening validation by checking if bundle.entry exists before accessing it.
2. The error message for invalid resource types could be more descriptive.

- throw new MetriportError(`Invalid resource type found`, undefined, {
+ throw new MetriportError(`Invalid resource type found when expecting ${resourceType}`, undefined, {
    invalidResourceType: invalidResource.resource.resourceType,
  });

Also, add a check for bundle.entry:

  const bundle = await this.makeRequest<FhirResourceBundle>({
    // ...
  });
+ if (!bundle.entry || !Array.isArray(bundle.entry)) {
+   return [];
+ }
  const invalidResource = bundle.entry.find(

packages/api/src/domain/resource-mapping-reversed.ts (2)

4-10: Type guard implementation could be improved.

The type guard function casts source to ResourceMappingReversedSource before checking if it's included, which could bypass TypeScript's type safety.

export function isResourceMappingReversedSource(
  source: string
): source is ResourceMappingReversedSource {
-  return resourceMappingSource.includes(source as ResourceMappingReversedSource);
+  return resourceMappingSource.includes(source as any);
}

---

12-19: Add documentation for domain type.

While the type is well-structured, it lacks documentation explaining what a "reversed" resource mapping represents. This would improve maintainability for new developers.

+/**
+ * Represents a mapping between a resource in the Metriport system and its corresponding 
+ * representation in an external system in a reversed direction (from external to Metriport).
+ */
export type ResourceMappingReversedPerSource = {
  externalId: string;
  cxId: string;
  patientId: string;
  patientMappingExternalId: string;
  resourceId: string;
  source: ResourceMappingReversedSource;
};

packages/api/src/external/ehr/canvas/command/start-resource-diff.ts (1)

14-38: Consider adding explicit error handling.

The function effectively orchestrates the resource diff process but relies on called functions for error handling. For improved robustness, consider adding specific error handling within this function.

export async function startCanvasResourceDiff({
  cxId,
  canvasPracticeId,
  canvasPatientId,
  direction,
}: StartResourceDiffParams): Promise<void> {
+  try {
    const existingPatient = await getPatientMappingOrFail({
      cxId,
      externalId: canvasPatientId,
      source: EhrSources.canvas,
    });
    const metriportPatient = await getPatientOrFail({
      cxId,
      id: existingPatient.patientId,
    });
    const ehrResourceDiffHandler = buildEhrStartResourceDiffHandler();
    await ehrResourceDiffHandler.startResourceDiff({
      ehr: EhrSources.canvas,
      cxId,
      practiceId: canvasPracticeId,
      metriportPatientId: metriportPatient.id,
      ehrPatientId: canvasPatientId,
      direction,
    });
+  } catch (error) {
+    throw new MetriportError("Failed to start Canvas resource diff", error as Error, {
+      cxId,
+      canvasPatientId,
+      direction
+    });
+  }
}

packages/lambdas/src/ehr-start-resource-diff.ts (2)

22-22: Consider avoiding arrow function for handler.

According to the coding guidelines, arrow functions should be avoided in modules. Converting this export to a traditional function expression or named function would align better with the project's guidelines.

-export const handler = Sentry.AWSLambda.wrapHandler(async (event: SQSEvent) => {
+export async function handler(event: SQSEvent) {

---

28-28: Use standardized logging interface.

The guidelines advise against using console.log outside of utils/infra/shared. Replace it with out().log, a custom logger, or prefixedLog for consistent logging, error tracking, and improved maintainability.

-    console.log(`Running with unparsed body: ${message.body}`);
+    log(`Running with unparsed body: ${message.body}`);

packages/shared/src/interface/external/ehr/fhir-resource.ts (1)

3-9: Consider stronger typing instead of z.any().

Although FHIR resources can vary widely, relying on z.any() bypasses type safety. Narrowing down the schema — or breaking it up if feasible — can yield earlier detection of incompatible resource fields during validation.

z.record(z.string(), z.any())
+// Potentially replace z.any() with respective FHIR field types or partials

packages/core/src/external/ehr/resource-diff/start/ehr-start-resource-diff-local.ts (1)

47-49: Rename loop variable for clarity.

When iterating over each sub-chunk, consider using a more descriptive name than chunk, which is the same name as the Lodash function. A concise rename can improve readability.

-for (const chunk of chunks) {
-  await Promise.all(chunk.map(params => computeResourceDiff(params)));
+for (const diffBatch of chunks) {
+  await Promise.all(diffBatch.map(params => computeResourceDiff(params)));

packages/infra/lib/ehr-nested-stack.ts (1)

58-76: New “startResourceDiff” configuration looks consistent.

You've reused the same pattern of queue & lambda config as in other setups. This method ensures a clear separation of concerns. Consider factoring out repeating logic if more diffs are expected in the future.

packages/api/src/command/mapping/resource-reversed.ts (2)

16-44: Upsert-like logic in createOrUpdateResourceMappingReversed.

This function updates an existing record or creates a new one if it doesn’t exist. If multiple concurrent requests for the same resource arrive, consider an explicit transaction or an upsert method to avoid duplicates. Otherwise, it looks good.

---

133-143: deleteAllResourceMappingReverseds for bulk deletion.

Function is straightforward. Consider returning a count of deleted rows to confirm the bulk operation if needed, but it’s optional.

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

Learnt from: leite08
PR: metriport/metriport#3598
File: packages/api/src/sequelize/migrations/2025-04-03_00_drop-cq-directory-entry.ts:96-99
Timestamp: 2025-04-03T18:27:13.730Z
Learning: When reviewing database migration files, the `down` function should exactly replicate the original table schema (without necessarily adding "improvements" like default values) to ensure proper rollback functionality, even if that schema doesn't follow all best practices.

packages/core/src/external/ehr/resource-diff/utils.ts (3)

40-149: Consider breaking down the complex function into smaller units

The computeResourceDiff function handles multiple responsibilities: validation, deduplication, and derivation checking. Consider refactoring it into smaller, more focused functions for better maintainability and testing.

Example refactoring approach:

function validateResourceTypes(
  existingResources: ResourceWithId[],
  newResource: ResourceWithId
): void {
  // Validation logic here
}

function deduplicateResources(
  resources: ResourceWithId[],
  resourceType: string
): Resource[] {
  // Deduplication switch logic here
}

export function computeResourceDiff({
  existingResources,
  newResource,
}: {
  existingResources: ResourceWithId[];
  newResource: ResourceWithId;
}): string[] {
  validateResourceTypes(existingResources, newResource);
  
  if (existingResources.some(resource => resource.id === newResource.id)) return [];
  
  const resources = existingResources.concat([newResource]);
  const deduplicatedResources = deduplicateResources(resources, newResource.resourceType);
  
  // Rest of the function...
}

---

151-157: Add documentation for the helper function

The resourceIsDerivedFromExistingResource function performs an important check, but its purpose and behavior aren't immediately clear. Consider adding a JSDoc comment to explain its purpose.

+/**
+ * Checks if a resource is derived from another resource by looking for
+ * a specific extension in the resource's extension array.
+ * 
+ * @param resource - The resource to check
+ * @returns true if the resource has the artifact related extension, false otherwise
+ */
function resourceIsDerivedFromExistingResource(resource: Resource): boolean {

---

152-152: Improve type safety with type guard

Instead of using in operator directly, consider using a proper type guard for better type safety and readability.

-  if (!("extension" in resource)) return false;
+  if (!resource.extension || !Array.isArray(resource.extension)) return false;

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

packages/api/src/command/mapping/resource-reversed.ts (4)

16-47: Consider relying on Sequelize’s built-in timestamps for updatedAt.

Manually setting existing.updatedAt = new Date(); alongside existing.changed("externalId", true); may be redundant if timestamps is enabled. Typically, assigning existing.externalId = externalId automatically marks the attribute as changed. You might simplify by removing these explicit updates and rely on Sequelize’s built-in behavior.

---

49-60: Prefer using .get({ plain: true }) instead of dataValues.

Returning dataValues works, but using .get({ plain: true }) or .toJSON() can improve clarity and avoid accidental references to the underlying Sequelize model fields.

---

117-134: Potential pagination.

If this endpoint is called for a large dataset, consider adding pagination or filtering to protect against excessive data loads.

---

136-146: Optional use of transactions or concurrency checks.

destroy queries all matching rows. If concurrency or partial updates are a concern, consider wrapping in a transaction or ensuring DB constraints align with your usage.

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

packages/core/src/external/ehr/resource-diff/utils.ts (4)

49-49: Fix typo in variable name

There's a typo in the variable name mewResourceType - it should be newResourceType.

-  const mewResourceType = newResource.resourceType;
+  const newResourceType = newResource.resourceType;

---

68-68: Same typo in switch statement

The same typo from above is used in the switch condition.

-  switch (mewResourceType) {
+  switch (newResourceType) {

---

141-141: Same typo in error message

The same typo appears in the error message template string.

-      throw new BadRequestError(`Unsupported resource type: ${mewResourceType}`, undefined, {
+      throw new BadRequestError(`Unsupported resource type: ${newResourceType}`, undefined, {

---

142-142: Same typo in error details

The typo also affects the error details object.

-        resourceType: mewResourceType,
+        resourceType: newResourceType,

packages/core/src/external/ehr/api/save-resource-diff.ts (1)

37-43: Consider potential issue with array joining.

When joining matchedResourceIds with commas, there could be potential issues if any of the IDs themselves contain commas.

Consider using a different delimiter or URL-safe encoding if you expect complex IDs:

-    matchedResourceIds: matchedResourceIds.join(","),
+    matchedResourceIds: JSON.stringify(matchedResourceIds), // Or another approach that handles special characters

packages/api/src/external/ehr/canvas/command/save-resource-diff.ts (1)

33-33: Fix typo in variable name.

There's a typo in the variable name fonud which should be found.

-    const fonud = matchedResourceIds.length > 0;
+    const found = matchedResourceIds.length > 0;

packages/shared/src/interface/external/ehr/fhir-resource.ts (1)

3-9: Well-structured schema definition with an important consideration

The schema definition using z.intersection allows for flexible FHIR resource definitions by combining fixed fields with an open record type. This approach is common for handling FHIR resources where certain properties are required while accommodating additional arbitrary properties.

However, be mindful that using z.any() in the record type removes type safety for those additional properties. While this flexibility is often necessary for FHIR resources, it may be worth considering if there are common optional properties that could be explicitly typed for better validation.

packages/core/src/external/ehr/canvas/index.ts (2)

43-59: Document the roadmap for supported resources

The array contains many commented-out resource types with only "DiagnosticReport" currently enabled. This suggests a phased implementation approach, but lacks documentation explaining the current state and future plans.

 export const supportedCanvasDiffResources = [
+  // Current implementation only supports DiagnosticReport.
+  // The commented resources below represent the planned roadmap for future support.
   /*
   "AllergyIntolerance",
   "Condition",
   */
   "DiagnosticReport",
   /*
   "Encounter",
   */
   //"Medication",
   /*
   "MedicationStatement",
   "MedicationRequest",
   //"Observation",
   "Procedure",
   */
 ];

---

373-416: Well-structured method with comprehensive error handling

The getFhirResourcesByResourceType method is well-implemented with proper parameter typing, URL construction, and error handling. It correctly handles the NotFoundError case by returning an empty array and properly validates the resource types in the response.

Two suggestions for improvement:

1. Consider adding a more descriptive error message in the MetriportError that includes the expected resource type
2. The error handling could be more specific to capture different error scenarios

 throw new MetriportError(`Invalid resource type found`, undefined, {
   invalidResourceType: invalidResource.resource.resourceType,
+  expectedResourceType: resourceType,
 });

 try {
   // ... existing code
 } catch (error) {
   if (error instanceof NotFoundError) return [];
+  if (error instanceof MetriportError) {
+    // Re-throw MetriportError instances (including our own validation errors)
+    throw error;
+  }
+  // Handle other types of errors (network, parsing, etc.)
+  throw new MetriportError(`Failed to fetch ${resourceType} resources`, error, additionalInfo);
 }

packages/core/src/external/ehr/resource-diff/utils.ts (5)

42-48: Add JSDoc comment to describe the function's purpose and return value

Add documentation to explain what this function does, what its parameters represent, and what the returned string array signifies.

+/**
+ * Computes the difference between existing resources and a new resource.
+ * Determines if a new resource should be kept after deduplication with existing resources.
+ * @param existingResources An array of existing resources with IDs
+ * @param newResource A new resource with ID to check against existing resources
+ * @returns An array of existing resource IDs if the new resource should be ignored, empty array if the new resource should be kept
+ */
 export function computeResourceDiff({
   existingResources,
   newResource,
 }: {
   existingResources: ResourceWithId[];
   newResource: ResourceWithId;
 }): string[] {

---

66-144: Extract deduplication logic to a separate function

The switch statement is lengthy and makes the function harder to read. Consider extracting it to a separate function for better readability and maintainability.

 const resources = existingResources.concat([newResource]);
-  let deduplicatedResources: Resource[];
-  switch (mewResourceType) {
-    case "AllergyIntolerance":
-      deduplicatedResources = deduplicateAllergyIntolerances(
-        resources as AllergyIntolerance[]
-      ).combinedResources;
-      break;
-    // ...other cases...
-    default:
-      throw new BadRequestError(`Unsupported resource type: ${mewResourceType}`, undefined, {
-        resourceType: mewResourceType,
-      });
-  }
+  const deduplicatedResources = deduplicateResources(resources, newResourceType);

// ... later in the file
+/**
+ * Deduplicates resources based on their resource type
+ * @param resources The resources to deduplicate
+ * @param resourceType The type of resources being deduplicated
+ * @returns The deduplicated resources
+ */
+function deduplicateResources(resources: Resource[], resourceType: string): Resource[] {
+  switch (resourceType) {
+    case "AllergyIntolerance":
+      return deduplicateAllergyIntolerances(
+        resources as AllergyIntolerance[]
+      ).combinedResources;
+    // ...other cases...
+    default:
+      throw new BadRequestError(`Unsupported resource type: ${resourceType}`, undefined, {
+        resourceType,
+      });
+  }
+}

---

158-164: Add JSDoc comment for helper function

Add documentation to explain the purpose of the resourceIsDerivedFromExistingResource function and what it checks for.

+/**
+ * Determines if a resource is derived from an existing resource by checking for a specific extension.
+ * @param resource The resource to check
+ * @returns True if the resource is derived from an existing resource, false otherwise
+ */
 function resourceIsDerivedFromExistingResource(resource: Resource): boolean {
   if (!("extension" in resource)) return false;
   const derivedFrom = resource.extension.find(
     extension => extension.url === artifactRelatedArtifactUrl
   );
   return derivedFrom !== undefined;
 }

---

159-159: Use truthy syntax for checking extensions

According to the coding guidelines, prefer truthy syntax instead of using the in operator.

-  if (!("extension" in resource)) return false;
+  if (!resource.extension) return false;

---

146-155: Consider adding more descriptive variable names

The return logic here is a bit hard to follow at first glance. Consider using more descriptive variable names to make the intent clearer.

   const existingResourceIds = existingResources.map(resource => resource.id);
-  const newResourceInDeduplicatedResources = deduplicatedResources.find(
+  const newResourceSurvivedDeduplication = deduplicatedResources.find(
     resource => resource.id === newResource.id
   );
-  if (!newResourceInDeduplicatedResources) return existingResourceIds;
+  if (!newResourceSurvivedDeduplication) return existingResourceIds;
   const newResourceIsDerivedFromExistingResource = resourceIsDerivedFromExistingResource(
-    newResourceInDeduplicatedResources
+    newResourceSurvivedDeduplication
   );
-  if (newResourceIsDerivedFromExistingResource) return existingResourceIds;
+  // If resource is derived, we should use existing resources
+  if (newResourceIsDerivedFromExistingResource) return existingResourceIds;
+  // New resource should be kept
   return [];

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

packages/core/src/external/ehr/canvas/index.ts (1)

367-410: Well-implemented FHIR resource retrieval method.

The getFhirResourcesByResourceType method follows good practices:

• Strong typing with the SupportedCanvasDiffResource type
• Proper URL parameter handling
• Resource validation to ensure correct types
• Appropriate error handling, particularly for NotFoundError
• Follows functional programming style with deterministic behavior

One minor suggestion for improvement:

Consider extracting the resource validation logic into a separate helper function to improve readability and maintainability:

  async getFhirResourcesByResourceType({
    cxId,
    patientId,
    resourceType,
    extraParams,
  }: {
    cxId: string;
    patientId: string;
    resourceType: SupportedCanvasDiffResource;
    extraParams?: Record<string, string>;
  }): Promise<FhirResources> {
    const { debug } = out(
      `Canvas getFhirResourcesByResourceType - cxId ${cxId} practiceId ${this.practiceId} patientId ${patientId} resourceType ${resourceType}`
    );
    const params = { ...(extraParams ?? {}), patient: `Patient/${patientId}` };
    const urlParams = new URLSearchParams(params);
    const resourceTypeUrl = `/${resourceType}?${urlParams.toString()}`;
    const additionalInfo = { cxId, practiceId: this.practiceId, patientId, resourceType };
    try {
      const bundle = await this.makeRequest<FhirResourceBundle>({
        cxId,
        patientId,
        s3Path: `fhir-resources-${resourceType}`,
        method: "GET",
        url: resourceTypeUrl,
        schema: fhirResourceBundleSchema,
        additionalInfo,
        debug,
        useFhir: true,
      });
-      const invalidResource = bundle.entry?.find(
-        resource => resource.resource.resourceType !== resourceType
-      );
-      if (invalidResource) {
-        throw new MetriportError(`Invalid resource type found`, undefined, {
-          invalidResourceType: invalidResource.resource.resourceType,
-        });
-      }
+      this.validateResourceTypes(bundle, resourceType);
      return bundle.entry?.map(resource => resource.resource) ?? [];
    } catch (error) {
      if (error instanceof NotFoundError) return [];
      throw error;
    }
  }
+
+  private validateResourceTypes(bundle: FhirResourceBundle, expectedType: string): void {
+    const invalidResource = bundle.entry?.find(
+      resource => resource.resource.resourceType !== expectedType
+    );
+    if (invalidResource) {
+      throw new MetriportError(`Invalid resource type found`, undefined, {
+        invalidResourceType: invalidResource.resource.resourceType,
+      });
+    }
+  }

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

packages/core/src/external/ehr/resource-diff/utils.ts (2)

68-68: ⚠️ Potential issue

Fix typo in variable name

There's a typo in the variable name mewResourceType - it should be newResourceType.

-  switch (mewResourceType) {
+  switch (newResourceType) {

---

140-144: ⚠️ Potential issue

Fix typo in error message and details

The same typo appears in the error message and context object.

-  throw new BadRequestError(`Unsupported resource type: ${mewResourceType}`, undefined, {
-    resourceType: mewResourceType,
+  throw new BadRequestError(`Unsupported resource type: ${newResourceType}`, undefined, {
+    resourceType: newResourceType,

packages/core/src/external/ehr/resource-diff/utils.ts (2)

158-164: Consider adding JSDoc comments for the helper function

The resourceIsDerivedFromExistingResource function is well-implemented, but adding JSDoc comments would improve code documentation and clarity.

+/**
+ * Determines if a resource is derived from an existing resource by checking
+ * its extensions for the artifact-related artifact URL.
+ * @param resource - The FHIR resource to check
+ * @returns true if the resource is derived from an existing resource
+ */
function resourceIsDerivedFromExistingResource(resource: Resource): boolean {

---

158-164: Consider using type predicate for safer property access

Using a type guard with a truthy check on resource.extension would be more type-safe than using "extension" in resource.

function resourceIsDerivedFromExistingResource(resource: Resource): boolean {
-  if (!("extension" in resource)) return false;
+  if (!resource.extension || !Array.isArray(resource.extension)) return false;
  const derivedFrom = resource.extension.find(
    extension => extension.url === artifactRelatedArtifactUrl
  );
  return derivedFrom !== undefined;
}

packages/lambdas/src/ehr-compute-resource-diff.ts (2)

17-22: Validate parseInt usage.
If the environment variable is invalid or non-numeric, parseInt may produce NaN. Ensure a check for NaN to avoid potential runtime issues.

 const waitTimeInMillis = parseInt(waitTimeInMillisRaw);
+ if (isNaN(waitTimeInMillis)) {
+   throw new MetriportError("Invalid WAIT_TIME_IN_MILLIS value", {
+     additionalInfo: { waitTimeInMillisRaw },
+   });
+ }

---

29-29: Use the recommended logging utility.
The coding guidelines advise against using console.log outside packages like infra or shared. Switch to a standardized logger such as out().log for consistency.

- console.log(`Running with unparsed body: ${message.body}`);
+ out().log(`Running with unparsed body: ${message.body}`);

- console.log(`${msg}: ${errorToString(error)}`);
+ out().log(`${msg}: ${errorToString(error)}`);

Also applies to: 45-45

packages/infra/lib/ehr-nested-stack.ts (2)

77-95: Use kebab-case for file entries.
The entry "ehr-Compute-resource-diff" uses uppercase “C.” Rename to align with the kebab-case standard.

- entry: "ehr-Compute-resource-diff",
+ entry: "ehr-compute-resource-diff",

---

369-426: Rename entry for compute resource diff.
Again, consider replacing the uppercase “C” in "ehr-Compute-resource-diff" to use kebab-case.

- entry: "ehr-Compute-resource-diff",
+ entry: "ehr-compute-resource-diff",

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

packages/core/src/external/ehr/resource-diff/utils.ts (3)

54-59: Consider adding more descriptive error messages

The current error messaging is brief. For better debuggability, consider adding more context about what the error means and potential resolution steps.

- throw new BadRequestError("Invalid existing resource types", undefined, {
+ throw new BadRequestError("Invalid existing resource types - all resources must have the same type as the new resource", undefined, {

---

68-144: Consider extracting the resource deduplication logic into a separate function

The switch statement is large and may grow as more resource types are supported. Consider refactoring this into a separate function to improve maintainability.

- let deduplicatedResources: Resource[];
- switch (newResourceType) {
-   case "AllergyIntolerance":
-     deduplicatedResources = deduplicateAllergyIntolerances(
-       resources as AllergyIntolerance[]
-     ).combinedResources;
-     break;
-   // ... other cases ...
- }
+ const deduplicatedResources = deduplicateResources(resources, newResourceType);

// Add this function below:
+ function deduplicateResources(resources: ResourceWithId[], resourceType: string): Resource[] {
+   switch (resourceType) {
+     case "AllergyIntolerance":
+       return deduplicateAllergyIntolerances(
+         resources as AllergyIntolerance[]
+       ).combinedResources;
+     // ... other cases ...
+   }
+ }

---

146-155: Logic looks good but could benefit from more explanatory comments

The logic that determines whether to return existing resource IDs or an empty array is crucial to the function's purpose but might not be immediately clear to maintainers. Consider adding a comment explaining the logic.

  const existingResourceIds = existingResources.map(resource => resource.id);
+ // If the new resource has been deduplicated away or is derived from existing ones,
+ // return the existing resource IDs to indicate the resource already exists
  const newResourceInDeduplicatedResources = deduplicatedResources.find(
    resource => resource.id === newResource.id
  );
  if (!newResourceInDeduplicatedResources) return existingResourceIds;
  const newResourceIsDerivedFromExistingResource = resourceIsDerivedFromExistingResource(
    newResourceInDeduplicatedResources
  );
  if (newResourceIsDerivedFromExistingResource) return existingResourceIds;
+ // Return an empty array to indicate this is a truly new resource
  return [];

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

packages/infra/lib/ehr-nested-stack.ts (1)

175-182: 🛠️ Refactor suggestion

Consider adding a RemovalPolicy to the S3 bucket.

The S3 bucket will default to RemovalPolicy.DESTROY, which could lead to accidental data loss during stack updates or removals. Since this bucket contains important EHR data, it's recommended to add a more protective removal policy.

import { Duration, NestedStack, NestedStackProps } from "aws-cdk-lib";
+import { RemovalPolicy } from "aws-cdk-lib";
import { SnsAction } from "aws-cdk-lib/aws-cloudwatch-actions";
import * as ec2 from "aws-cdk-lib/aws-ec2";
import { Function as Lambda } from "aws-cdk-lib/aws-lambda";
import { SqsEventSource } from "aws-cdk-lib/aws-lambda-event-sources";
import * as s3 from "aws-cdk-lib/aws-s3";
import { Queue } from "aws-cdk-lib/aws-sqs";
import { Construct } from "constructs";
// ...

const ehrBundleBucket = new s3.Bucket(this, "EhrBundleBucket", {
  bucketName: props.config.ehrBundleBucketName,
  publicReadAccess: false,
  encryption: s3.BucketEncryption.S3_MANAGED,
  versioned: true,
+  removalPolicy: RemovalPolicy.RETAIN,
});

packages/infra/lib/ehr-nested-stack.ts (6)

94-96: Fix variable naming inconsistency.

The variable ComputeResourceDiffBundlesLambdaTimeout uses PascalCase instead of camelCase, which is inconsistent with other similar variables in the codebase and violates the naming convention specified in your coding guidelines.

-const ComputeResourceDiffBundlesLambdaTimeout = waitTimeComputeResourceDiff.plus(
+const computeResourceDiffBundlesLambdaTimeout = waitTimeComputeResourceDiff.plus(
   Duration.minutes(5)
 );

---

120-120: Maintain naming consistency for timeout variables.

Similarly, keep the same naming pattern for all timeout variables.

-const RefreshEhrBundlesLambdaTimeout = waitTimeRefreshBundle.plus(Duration.minutes(5));
+const refreshEhrBundlesLambdaTimeout = waitTimeRefreshBundle.plus(Duration.minutes(5));

---

100-103: Update Lambda timeout reference in configuration.

Make sure to update the reference to the timeout variable in the Lambda settings.

   lambda: {
     memory: 1024,
-    timeout: ComputeResourceDiffBundlesLambdaTimeout,
+    timeout: computeResourceDiffBundlesLambdaTimeout,
   },

---

125-127: Update Lambda timeout reference in configuration.

Same issue with the refreshEhrBundles Lambda settings.

   lambda: {
     memory: 1024,
-    timeout: RefreshEhrBundlesLambdaTimeout,
+    timeout: refreshEhrBundlesLambdaTimeout,
   },

---

108-110: Update Queue visibility timeout reference.

Update the reference to the timeout variable in the queue configuration.

     visibilityTimeout: Duration.seconds(
-      ComputeResourceDiffBundlesLambdaTimeout.toSeconds() * 2 + 1
+      computeResourceDiffBundlesLambdaTimeout.toSeconds() * 2 + 1
     ),

---

132-132: Update Queue visibility timeout reference.

Update the reference to the timeout variable in the refreshEhrBundles queue configuration.

-    visibilityTimeout: Duration.seconds(RefreshEhrBundlesLambdaTimeout.toSeconds() * 2 + 1),
+    visibilityTimeout: Duration.seconds(refreshEhrBundlesLambdaTimeout.toSeconds() * 2 + 1),

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