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

30-36: Route documentation caution.
Coding guidelines suggest focusing comments on the rationale behind design decisions, rather than describing how code works. Route documentation is acceptable, but consider refining comments to highlight “why” it’s done rather than simply “what” or “how”.

---

37-52: Consider returning a structured JSON response.
Returning the raw ID can be inconsistent with typical REST endpoints, which often return an object. Suggest:

-return res.status(httpStatus.OK).json(patientId);
+return res.status(httpStatus.OK).json({ patientId });

packages/lambdas/src/shared/oss-api.ts (1)

29-29: Consider a custom response type.
Returning a plain AxiosResponse can obscure the result shape. A named interface for the response structure would improve clarity.

packages/lambdas/src/ehr-sync-patient.ts (1)

23-73: Redundant condition in error handling.
Lines 52–53 repeat (error.response?.status === 404) twice. Suggest simplifying:

-if (
-  isAxiosError(error) &&
-  (error.response?.status === 404 || error.response?.status === 404)
-) {
+if (
+  isAxiosError(error) &&
+  error.response?.status === 404
+) {

packages/api/src/external/ehr/elation/command/process-patients-from-appointments.ts (2)

161-166: Constructing JSON payload
Building a minimal sync payload is clear and maintainable. For future extensibility, consider including additional metadata (e.g., timestamp) to aid debugging if messages fail.

---

175-175: Error handling after message send
The catch block logs errors if sending to SQS fails. As a refinement, consider also capturing the SQS response or partial batch items for more contextual troubleshooting.

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

93-150: setupSyncPatient method
• Creates a FIFO queue and attaches the Lambda with event source mapping as intended.
• The naming and environment variables are clear.
• Consider verifying that the queue’s visibilityTimeout is set appropriately based on your Lambda’s maximum runtime, especially with partial batch responses.

packages/api/src/external/ehr/athenahealth/command/process-patients-from-appointments-sub.ts (1)

202-202: Error logging
Logging on queue failure is helpful for diagnosing issues. If needed, consider capturing more details (e.g., partial batch item failures).

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

packages/api/src/external/ehr/shared.ts (1)

19-19: Check concurrency overhead
Raising parallelPatients from a much lower number to 200 can overload external APIs if they cannot handle this level of concurrency. Ensure a suitable backoff strategy or error handling is in place to address rate limits or transient failures.

packages/api/src/external/ehr/athenahealth/command/process-patients-from-appointments-sub.ts (1)

189-203: Add retry or resilience strategy for queue operations
Transitioning to sending patient sync requests to SQS decouples the workflow, which is great. However, if sendMessageToQueue fails, the call is only logged. Consider adding retries or implementing a queue fallback path to safeguard against transient SQS or network issues.

packages/api/src/external/ehr/elation/command/process-patients-from-appointments.ts (2)

109-109: Potential EHR system load
Running large batches in parallel with minimal delay may overwhelm external EHR endpoints. Confirm if the external API imposes concurrency limits or rate throttles, and consider adaptive throttling to prevent failures.

---

157-157: Queue-based patient sync
Shifting to an SQS-based flow is a more scalable design. However, including a retry policy or post-queue verification would help ensure message reliability. Also, verify that removing triggerDq does not affect downstream logic that relies on data quality checks.

Also applies to: 162-162, 164-167, 168-176

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

packages/lambdas/src/ehr-sync-patient.ts (1)

37-61: 🛠️ Refactor suggestion

Simplify error handling by removing nested try/catch.

Since there's no loop here, the nested try/catch blocks create unnecessary complexity. Consider merging them into a single error handling block.

-    try {
      log(`Parsed: ${JSON.stringify(parsedBody)}, waitTimeInMillis ${waitTimeInMillis}`);

      const processSyncPatientRequest: ProcessSyncPatientRequest = {
        ehr,
        cxId,
        practiceId,
        patientId,
        triggerDq,
      };
      const ehrSyncPatientHandler = new EhrSyncPatientLocal(waitTimeInMillis);

      await ehrSyncPatientHandler.processSyncPatient(processSyncPatientRequest);

      const finishedAt = new Date().getTime();
      log(`Done local duration: ${finishedAt - startedAt}ms`);
-    } catch (error) {
-      errorHandled = true;
-      capture.error(errorMsg, {
-        extra: { event, context: lambdaName, error },
-      });
-      throw new MetriportError(errorMsg, error, {
-        ...{ ...parsedBody, patientPayload: undefined },
-      });
-    }

packages/lambdas/src/shared/ehr.ts (2)

11-33: Simplify validation logic with a cleaner pattern

Consider refactoring the validation logic to reduce repetition and improve maintainability. You could implement a validation utility function to handle common validation patterns.

-  const cxIdRaw = bodyAsJson.cxId;
-  if (!cxIdRaw) throw new Error(`Missing cxId`);
-  if (typeof cxIdRaw !== "string") throw new Error(`Invalid cxId`);
-
-  const ehrRaw = bodyAsJson.ehr;
-  if (!ehrRaw) throw new Error(`Missing ehr`);
-  if (typeof ehrRaw !== "string") throw new Error(`Invalid ehr`);
-  if (!isEhrSource(ehrRaw)) throw new Error(`Invalid ehr`);
-
-  const practiceIdRaw = bodyAsJson.practiceId;
-  if (!practiceIdRaw) throw new Error(`Missing practiceId`);
-  if (typeof practiceIdRaw !== "string") throw new Error(`Invalid practiceId`);
-
-  const patientIdRaw = bodyAsJson.patientId;
-  if (!patientIdRaw) throw new Error(`Missing patientId`);
-  if (typeof patientIdRaw !== "string") throw new Error(`Invalid patientId`);
-
-  const triggerDqRaw = bodyAsJson.triggerDq;
-  if (triggerDqRaw === undefined) throw new Error(`Missing triggerDq`);
-  if (typeof triggerDqRaw !== "boolean") throw new Error(`Invalid triggerDq`);
+  const validateString = (value: unknown, fieldName: string): string => {
+    if (!value) throw new Error(`Missing ${fieldName}`);
+    if (typeof value !== "string") throw new Error(`Invalid ${fieldName}`);
+    return value;
+  };
+
+  const cxIdRaw = validateString(bodyAsJson.cxId, "cxId");
+  const ehrRaw = validateString(bodyAsJson.ehr, "ehr");
+  if (!isEhrSource(ehrRaw)) throw new Error(`Invalid ehr`);
+  const practiceIdRaw = validateString(bodyAsJson.practiceId, "practiceId");
+  const patientIdRaw = validateString(bodyAsJson.patientId, "patientId");
+  
+  const triggerDqRaw = bodyAsJson.triggerDq;
+  if (triggerDqRaw === undefined) throw new Error(`Missing triggerDq`);
+  if (typeof triggerDqRaw !== "boolean") throw new Error(`Invalid triggerDq`);

---

28-30: Ensure consistent validation pattern

The validation for triggerDq uses a different pattern than the string validations (=== undefined vs !value). Consider making this consistent with the rest of the validation logic.

packages/core/src/command/ehr/sync-patient/ehr-sync-patient-factory.ts (1)

6-13: Add error handling for missing queue URL

The function returns EhrSyncPatientCloud with the queue URL in non-dev environments, but doesn't handle the case where the URL might be missing or invalid. Consider adding validation or error handling.

  if (Config.isDev()) {
    const waitTimeAtTheEndInMillis = 0;
    return new EhrSyncPatientLocal(waitTimeAtTheEndInMillis);
  }
  const ehrSyncPatientQueueUrl = Config.getEhrSyncPatientQueueurl("");
+  if (!ehrSyncPatientQueueUrl) {
+    throw new Error("Missing EHR sync patient queue URL in non-dev environment");
+  }
  return new EhrSyncPatientCloud(ehrSyncPatientQueueUrl);

packages/core/src/command/ehr/sync-patient/ehr-sync-patient-local.ts (4)

17-19: Avoid multi-line logs

According to the coding guidelines, multi-line logs should be avoided. Consider simplifying this log statement.

-    const { log } = out(
-      `processSyncPatient.local - ehr ${ehr} cxId ${cxId} practiceId ${practiceId} patientId ${patientId}`
-    );
+    const { log } = out("processSyncPatient.local");
+    log(`Processing: ehr=${ehr}, cxId=${cxId}, practiceId=${practiceId}, patientId=${patientId}`);

---

31-33: Follow error messaging guidelines

According to the coding guidelines, "Error messages should have a static message - add dynamic data to MetriportError's additionalInfo prop". Consider using a MetriportError with static message and additional context.

-      const msg = `Failure while processing sync patient @ Ehr`;
-      log(`${msg}. Cause: ${errorToString(error)}`);
+      const msg = "Failure while processing sync patient @ Ehr";
+      log(`${msg}. Cause: ${errorToString(error)}`);
+      // Consider using MetriportError with additionalInfo for context if applicable

---

7-46: Consider dependency injection for better testability

The class directly depends on the syncPatient function, which makes testing harder. Consider injecting this dependency in the constructor for better testability.

  export class EhrSyncPatientLocal implements EhrSyncPatientHandler {
-  constructor(private readonly waitTimeInMillis: number) {}
+  constructor(
+    private readonly waitTimeInMillis: number,
+    private readonly syncPatientFn = syncPatient
+  ) {}

  async processSyncPatient({
    ehr,
    cxId,
    practiceId,
    patientId,
    triggerDq,
  }: ProcessSyncPatientRequest): Promise<void> {
    // ...
    try {
-      await syncPatient({
+      await this.syncPatientFn({
        ehr,
        cxId,
        practiceId,
        patientId,
        triggerDq,
      });
      // ...

---

30-44: Prevent duplicate Sentry events for the same error

According to the coding guidelines, you should "Avoid sending multiple events to Sentry for a single error". The current implementation captures the error and then throws it, which might result in multiple Sentry captures if caught and captured again by upstream handlers.

Consider adding a flag to the error to indicate it has already been captured, or adjust the error handling strategy.

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

29-52: Consider URL path/query parameter inconsistency

The endpoint is defined as /:id in the URL path, but the implementation doesn't use this path parameter, instead relying on patientId from the query string. This creates a confusing API contract where the same identifier is potentially passed twice in different locations.

Consider either:

1. Using the path parameter: req.params.id instead of requiring a separate query parameter
2. Changing the route to a more descriptive one without a path parameter

Additionally, the endpoint always returns 200 OK regardless of whether the sync operation is successful. This is acceptable for asynchronous processing but consider adding a more informative response payload.

-router.post(
-  "/:id",
-  requestLogger,
-  asyncHandler(async (req: Request, res: Response) => {
-    const cxId = getUUIDFrom("query", req, "cxId").orFail();
-    const canvasPatientId = getFromQueryOrFail("patientId", req);
-    const canvasPracticeId = getFromQueryOrFail("practiceId", req);
-    const triggerDq = getFromQueryAsBoolean("triggerDq", req);
-    syncCanvasPatientIntoMetriport({
-      cxId,
-      canvasPracticeId,
-      canvasPatientId,
-      triggerDq,
-    }).catch(processAsyncError("Canvas syncCanvasPatientIntoMetriport"));
-    return res.sendStatus(httpStatus.OK);
-  })
-);
+router.post(
+  "/:id",
+  requestLogger,
+  asyncHandler(async (req: Request, res: Response) => {
+    const cxId = getUUIDFrom("query", req, "cxId").orFail();
+    const canvasPatientId = req.params.id;
+    const canvasPracticeId = getFromQueryOrFail("practiceId", req);
+    const triggerDq = getFromQueryAsBoolean("triggerDq", req);
+    syncCanvasPatientIntoMetriport({
+      cxId,
+      canvasPracticeId,
+      canvasPatientId,
+      triggerDq,
+    }).catch(processAsyncError("Canvas syncCanvasPatientIntoMetriport"));
+    return res.sendStatus(httpStatus.ACCEPTED);
+  })
+);

packages/api/src/external/ehr/shared.ts (1)

33-33:

❓ Verification inconclusive

Verify impact of increased parallelism

The parallelPatients constant has been increased significantly to 200. While this will improve throughput, verify that downstream systems can handle this level of concurrent requests.

Consider monitoring system performance after deployment to ensure that this increased parallelism doesn't overwhelm downstream systems. You might want to gradually increase this value in production rather than jumping directly to 200.

---

🏁 Script executed:

#!/bin/bash
# Check for any rate limiting configuration or discussions in the codebase

rg -A 3 "rate.*limit|throttl" --type ts

Length of output: 10881

---

🏁 Script executed:

#!/bin/bash
# Search for usage of "parallelPatients" across the repository
rg "parallelPatients" -n

Length of output: 844

---

Action: Verify Impact of Increased Parallelism

The parallelPatients constant is now set to 200 in packages/api/src/external/ehr/shared.ts (Line 33) and is used in multiple commands (e.g., in the Elation, Canvas, and AthenaHealth modules). Given that our current rate limiting configurations (such as the burst and rate limits defined in our infra stack) remain on relatively lower thresholds, please:

• Verify that downstream systems can handle 200 concurrent executions without being overwhelmed.
• Review the integration points in:
  • packages/api/src/external/ehr/elation/command/process-patients-from-appointments.ts
  • packages/api/src/external/ehr/canvas/command/process-patients-from-appointments.ts
  • packages/api/src/external/ehr/athenahealth/command/process-patients-from-appointments.ts
• Consider performing a gradual rollout or stress testing in a staging environment, and closely monitor system performance post-deployment to ensure that existing rate limiting settings (e.g., those set to burstLimit: 10 and rateLimit: 50) adequately support the increased parallelism.

packages/lambdas/src/ehr-sync-patient.ts (3)

21-22: Consider implementing Sentry's handler pattern.

As mentioned in the past review comment, there's a PR to move all lambdas to use Sentry's handler pattern. Consider implementing this pattern for consistency.

- // Don't use Sentry's default error handler b/c we want to use our own and send more context-aware data
+ // Using our own error handler pattern to provide more context-aware data

---

28-28: Don't early return after getting message.

The early return on line 28 makes the code harder to follow. Consider restructuring to use an if-else pattern instead.

-    if (!message) return;
+    if (message) {
+      // The rest of the processing code
+    }

---

72-101: Optimize the parseBody function.

The parseBody function can be simplified and made more robust by reducing variable redundancy and improving type checking.

 function parseBody(body?: unknown): {
   cxId: string;
   ehr: EhrSource;
   practiceId: string;
   patientId: string;
   triggerDq: boolean;
 } {
   if (!body) throw new Error(`Missing message body`);

   const bodyString = typeof body === "string" ? (body as string) : undefined;
   if (!bodyString) throw new Error(`Invalid body`);

   const bodyAsJson = JSON.parse(bodyString);

   const { cxIdRaw, ehrRaw, practiceIdRaw, patientIdRaw, triggerDqRaw } = parseEhrIds(bodyAsJson);

-  const cxId = cxIdRaw;
-  const ehr = ehrRaw;
-  const practiceId = practiceIdRaw;
-  const patientId = patientIdRaw;
-  const triggerDq = triggerDqRaw;
-
   return {
-    cxId,
-    ehr,
-    practiceId,
-    patientId,
-    triggerDq,
+    cxId: cxIdRaw,
+    ehr: ehrRaw,
+    practiceId: practiceIdRaw,
+    patientId: patientIdRaw,
+    triggerDq: triggerDqRaw,
   };
 }

packages/core/src/command/ehr/sync-patient/ehr-sync-patient-cloud.ts (1)

15-43: Improve error handling with more specific error messages.

The error message in the catch block is generic and could be more descriptive to aid in debugging. Consider adding more context about what specifically failed.

   async processSyncPatient(params: ProcessSyncPatientRequest): Promise<void> {
     const { ehr, cxId, practiceId, patientId, triggerDq } = params;
     const { log } = out(
       `processSyncPatient.cloud - ehr ${ehr} cxId ${cxId} practiceId ${practiceId} patientId ${patientId}`
     );
     try {
       const payload = JSON.stringify(params);
       await sqsClient.sendMessageToQueue(this.ehrSyncPatientQueueUrl, payload, {
         fifo: true,
         messageDeduplicationId: createUuidFromText(payload),
         messageGroupId: cxId,
       });
     } catch (error) {
-      const msg = `Failure while processing patient sync @ Ehr`;
+      const msg = `Failed to send patient sync message to queue for ehr ${ehr}`;
       log(`${msg}. Cause: ${errorToString(error)}`);
       capture.error(msg, {
         extra: {
           ehr,
           cxId,
           practiceId,
           patientId,
           triggerDq,
           context: "ehr-sync-patient-cloud.processSyncPatient",
           error,
         },
       });
       throw error;
     }
   }

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

packages/lambdas/src/ehr-sync-patient.ts (2)

46-53: 🛠️ Refactor suggestion

Use Sentry.setExtras() instead of capture.error() to avoid duplicate events.

According to the past review comment, you should use Sentry.setExtras() to avoid creating duplicate events and then throw a wrapped error.

+import * as Sentry from "@sentry/serverless";

      errorHandled = true;
-      capture.error(errorMsg, {
-        extra: { event, context: lambdaName, error },
-      });
+      Sentry.setExtras({ event, context: lambdaName, error });
       throw new MetriportError(errorMsg, error, {
         ...{ ...parsedBody, patientPayload: undefined },
       });

---

56-59: 🛠️ Refactor suggestion

Use Sentry.setExtras() in the outer catch block as well.

Apply the same Sentry pattern to the outer catch block for consistency.

    if (errorHandled) throw error;
    console.log(`${errorMsg}: ${errorToString(error)}`);
-    capture.error(errorMsg, {
-      extra: { event, context: lambdaName, error },
-    });
+    Sentry.setExtras({ event, context: lambdaName, error });
    throw new MetriportError(errorMsg, error);

packages/lambdas/src/shared/ehr.ts (2)

4-10: Improve naming by removing unnecessary 'Raw' suffixes.

The 'Raw' suffix in the return type is misleading because by the time the function returns, the values are already validated.

export function parseSyncPatient(bodyAsJson: any): {
-  cxIdRaw: string;
-  ehrRaw: EhrSource;
-  practiceIdRaw: string;
-  patientIdRaw: string;
-  triggerDqRaw: boolean;
+  cxId: string;
+  ehr: EhrSource;
+  practiceId: string;
+  patientId: string;
+  triggerDq: boolean;

---

11-33: Refactor to create a more DRY validation approach.

The current implementation repeats similar validation logic for each field. Consider refactoring to make the code more DRY:

-  const cxIdRaw = bodyAsJson.cxId;
-  if (!cxIdRaw) throw new Error(`Missing cxId`);
-  if (typeof cxIdRaw !== "string") throw new Error(`Invalid cxId`);
-
-  const ehrRaw = bodyAsJson.ehr;
-  if (!ehrRaw) throw new Error(`Missing ehr`);
-  if (typeof ehrRaw !== "string") throw new Error(`Invalid ehr`);
-  if (!isEhrSource(ehrRaw)) throw new Error(`Invalid ehr`);
-
-  const practiceIdRaw = bodyAsJson.practiceId;
-  if (!practiceIdRaw) throw new Error(`Missing practiceId`);
-  if (typeof practiceIdRaw !== "string") throw new Error(`Invalid practiceId`);
-
-  const patientIdRaw = bodyAsJson.patientId;
-  if (!patientIdRaw) throw new Error(`Missing patientId`);
-  if (typeof patientIdRaw !== "string") throw new Error(`Invalid patientId`);
-
-  const triggerDqRaw = bodyAsJson.triggerDq;
-  if (triggerDqRaw === undefined) throw new Error(`Missing triggerDq`);
-  if (typeof triggerDqRaw !== "boolean") throw new Error(`Invalid triggerDq`);
-
-  return { cxIdRaw, ehrRaw, practiceIdRaw, patientIdRaw, triggerDqRaw };
+  const payload = bodyAsJson as Record<string, unknown>;
+  
+  function validateString(field: string, value: unknown): string {
+    if (!value) throw new Error(`Missing ${field}`);
+    if (typeof value !== "string") throw new Error(`Invalid ${field}`);
+    return value;
+  }
+  
+  const cxId = validateString("cxId", payload.cxId);
+  
+  const ehrString = validateString("ehr", payload.ehr);
+  if (!isEhrSource(ehrString)) throw new Error(`Invalid ehr`);
+  const ehr = ehrString;
+  
+  const practiceId = validateString("practiceId", payload.practiceId);
+  const patientId = validateString("patientId", payload.patientId);
+  
+  const triggerDq = payload.triggerDq;
+  if (triggerDq === undefined) throw new Error(`Missing triggerDq`);
+  if (typeof triggerDq !== "boolean") throw new Error(`Invalid triggerDq`);
+  
+  return { cxId, ehr, practiceId, patientId, triggerDq };

packages/lambdas/src/ehr-sync-patient.ts (4)

36-53: Simplify error handling structure.

There's no loop, so having nested try/catch blocks is unnecessarily complex. Consider consolidating:

-    try {
      log(`Parsed: ${JSON.stringify(parsedBody)}, waitTimeInMillis ${waitTimeInMillis}`);

      const ehrSyncPatientHandler = new EhrSyncPatientLocal(waitTimeInMillis);

      await ehrSyncPatientHandler.processSyncPatient(parsedBody);

      const finishedAt = new Date().getTime();
      log(`Done local duration: ${finishedAt - startedAt}ms`);
-    } catch (error) {
-      errorHandled = true;
-      capture.error(errorMsg, {
-        extra: { event, context: lambdaName, error },
-      });
-      throw new MetriportError(errorMsg, error, {
-        ...{ ...parsedBody, patientPayload: undefined },
-      });
-    }

---

29-29: Use prefixed logging instead of console.log.

According to your coding guidelines, you should avoid using console.log directly. Use the prefixed log for consistency:

-    console.log(`Running with unparsed body: ${message.body}`);
+    const log = prefixedLog(
+      `ehr-sync-patient`
+    );
+    log(`Running with unparsed body: ${message.body}`);

---

75-79: Remove redundant variable assignments.

These assignments are simply renaming variables without any transformation, which is unnecessary and can be confusing:

-  const cxId = cxIdRaw;
-  const ehr = ehrRaw;
-  const practiceId = practiceIdRaw;
-  const patientId = patientIdRaw;
-  const triggerDq = triggerDqRaw;
-
   return {
-    cxId,
-    ehr,
-    practiceId,
-    patientId,
-    triggerDq,
+    cxId: cxIdRaw,
+    ehr: ehrRaw,
+    practiceId: practiceIdRaw,
+    patientId: patientIdRaw,
+    triggerDq: triggerDqRaw,
   };

---

67-68: Improve type handling for message body.

The current type casting could be improved with better error handling and type narrowing:

-  const bodyString = typeof body === "string" ? (body as string) : undefined;
-  if (!bodyString) throw new Error(`Invalid body`);
+  if (typeof body !== "string") {
+    throw new MetriportError("Invalid message body type", undefined, { 
+      bodyType: typeof body 
+    });
+  }
+  const bodyString = body;

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