packages/shared/src/domain/patient/patient-import/schemas.ts (1)

29-34: 🛠️ Refactor suggestion

Schema now allows empty objects – refine to require at least one prop

Since status is now optional the following passes validation but is useless:

updateJobSchema.parse({})

Guard it at the schema level:

 export const updateJobSchema = z
   .object({
     status: z.enum(patientImportJobStatus).optional(),
     total: z.number().optional(),
     failed: z.number().optional(),
     forceStatusUpdate: z.boolean().optional(),
-});
+})
+.refine(
+  d => d.status !== undefined || d.total !== undefined || d.failed !== undefined,
+  { message: "At least one of status, total or failed must be provided" },
+);

Pushing the constraint into zod removes the need for scattered runtime checks.

packages/core/src/command/patient-import/steps/create/patient-import-create-cloud.ts (1)

8-11: Same DI concern as in patient-import-query-cloud

Inline creation of SQSClient limits testability; consider accepting it as a constructor argument with a sensible default.

packages/core/src/command/patient-import/steps/query/patient-import-query-local.ts (1)

43-45: Same silent-failure pattern as other Local wrappers – see earlier comment; considerations there apply here as well.

packages/core/src/command/patient-import/steps/create/patient-import-create-local.ts (1)

41-43: Same silent-failure pattern as other Local wrappers – see earlier comment; considerations there apply here as well.

packages/core/src/command/patient-import/csv/store-results.ts (1)

24-26: reasonForDev is never consumed – dead field or missing plumbing?

ResultEntry now carries both reasonForCx and reasonForDev, but only the customer-facing reason is ever read in this module. If developer reasons are needed elsewhere, wire them through; otherwise drop the unused property to avoid type-rot and larger payloads.

packages/lambdas/src/patient-import-parse.ts (1)

30-34: Explicitly pass next/result when diverging from defaults

processJobParse allows overriding next and result. If the lambda ever needs custom wiring (e.g., for A/B or test doubles) the current implicit defaults hide this option. Consider passing them explicitly or documenting why defaults are always sufficient.

packages/core/src/command/patient-import/steps/query/patient-import-query-factory.ts (1)

7-11: Constructor now parameter-less – re-check testability & env flexibility

The factory no longer injects patientImportBucket / queueUrl.
Great for brevity, but:

1. Hard-wiring configs inside the classes makes unit testing harder (no DI).
2. Alternate environments (staging with different buckets) now require code changes instead of config.

If the classes read from Config internally, consider still accepting overrides via constructor for flexibility.

packages/lambdas/src/patient-import-result.ts (1)

29-34: Variable name doesn’t reflect the new command

processJobResultCmd still carries the old “job-result” wording, which can be misleading now that we’re invoking processPatientResult. Renaming improves clarity and greppability.

-      const processJobResultCmd: ProcessPatientResultCommandRequest = {
+      const processPatientResultCmd: ProcessPatientResultCommandRequest = {
         cxId,
         jobId,
         patientImportBucket,
       };
-      await processPatientResult(processJobResultCmd);
+      await processPatientResult(processPatientResultCmd);

packages/lambdas/src/patient-import-query.ts (2)

26-29: Specify radix in parseInt to avoid accidental octal parsing

parseInt(waitTimeInMillisRaw) will treat leading zeros as octal on some runtimes. Make the intent explicit:

-const waitTimeInMillis = parseInt(waitTimeInMillisRaw);
+const waitTimeInMillis = parseInt(waitTimeInMillisRaw, 10);

---

38-50: Replace console.log with the standard logger

Coding-guidelines forbid console.log outside utils/infra/shared packages. Use prefixedLog (already available in this scope) or out().log to remain consistent and keep structured logging.

-    console.log(`Running with unparsed body: ${message.body}`);
+    prefixedLog(lambdaName)(`Running with unparsed body: ${message.body}`);
@@
-    console.log(`Done local duration: ${finishedAt - startedAt}ms`);
+    prefixedLog(lambdaName)(`Done local duration: ${finishedAt - startedAt}ms`);

packages/lambdas/src/patient-import-create.ts (1)

38-39: Replace console.log with the project’s structured logger

Coding guidelines restrict console.* usage outside utils/infra/shared.
You already have prefixedLog – use it (or out().log) instead to keep log
formatting consistent and avoid missing correlation IDs in production logs.

Also applies to: 74-75

packages/shared/src/common/date.ts (1)

28-28: Redundant condition

!date is checked twice in the same clause; remove the duplication to keep the guard concise.

packages/shared/src/common/__tests__/date.test.ts (2)

173-179: Duplicate test description

There are two it("returns true when date is valid ISO datetime", …) blocks. Rename one of
them to avoid confusion when reading test output.

---

128-128: Remove stray console.log

console.log(result); will pollute CI logs and is not needed for assertions.

packages/core/src/command/patient-import/csv/__tests__/convert-patient.test.ts (1)

165-169: Spy does not stub return value

jest.spyOn(normalizeDobFile, "normalizeDobSafe") only records calls; if
normalizeDobSafe throws the same failure will propagate into the test.
Be explicit (mockReturnValueOnce(...)) where the function’s behaviour matters to keep
tests deterministic.

packages/core/src/command/patient-import/steps/parse/patient-import-parse-cloud.ts (1)

7-10: Constructor default creates a new client per instance

Inlining makeLambdaClient in the parameter default means every new
PatientImportParseCloud() instantiation spins up an AWS SDK client.
If these objects are short-lived, this may create unnecessary sockets.
Consider injecting a shared client from the caller or making the default a
module-level singleton.

packages/core/src/command/patient-import/steps/result/patient-import-result-cloud.ts (1)

7-10: Same client-instantiation concern as parse step

See previous comment – apply the same singleton / DI pattern here for consistency.

packages/core/src/command/patient-import/steps/query/patient-import-query-cloud.ts (1)

7-10: Prefer dependency injection over inline instantiation

Creating a fresh SQSClient inside the constructor ties the class to AWS at instantiation time and makes unit-testing harder. Passing an already-built client (with the same default) keeps the current DX while allowing callers to inject stubs/mocks when needed.

packages/core/src/command/patient-import/steps/query/patient-import-query-command.ts (2)

52-53: Guard against negative wait times

if (waitTimeAtTheEndInMillis > 0) … silently ignores negatives; clamping would be clearer:

-if (waitTimeAtTheEndInMillis > 0) await sleep(waitTimeAtTheEndInMillis);
+await sleep(Math.max(0, waitTimeAtTheEndInMillis));

---

13-16: Avoid extending DayJS plugin on every import

dayjs.extend(duration); executes each time this module is required. Moving the extension to a central bootstrap file prevents accidental multiple calls and keeps plugin setup in one place.

packages/api/src/routes/internal/medical/patient-import.ts (1)

302-304: Make the validation message explicit

Including the hard limit helps the caller immediately understand why the request was rejected:

-throw new BadRequestError(`Debug mode is not supported for jobs with many patients`);
+throw new BadRequestError(
+  `Debug mode is limited to ${maxPatientsOnDebugGetBulk} patients or fewer`
+);

packages/core/src/command/patient-import/steps/create/patient-import-create-command.ts (1)

59-65: Update only the fields that changed to avoid stale writes

Re-passing the entire patientRecord object can unintentionally overwrite fields changed elsewhere after the first write. Prefer a minimal patch:

-  await Promise.all([
-    updatePatientRecord({
-      ...patientRecord,
-      patientId,
-      bucketName: patientImportBucket,
-    }),
+  await Promise.all([
+    updatePatientRecord({
+      cxId,
+      jobId,
+      rowNumber,
+      patientId,
+      bucketName: patientImportBucket,
+    }),

This keeps the write explicit and decoupled from the record’s full interface.

packages/core/src/command/patient-import/steps/result/patient-import-result-command.ts (4)

13-13: Remove unused dayjs.duration plugin import

dayjs.extend(duration); is never referenced afterwards, so the plugin isn’t needed here and can be deleted to keep dependencies lean.

---

66-71: Shared array mutation inside concurrent loop can cause unpredictable ordering

records.push(patientRecord) from multiple async tasks means the final array order depends on fetch-completion order, not the original CSV order.
If consumer code relies on row sequence, preserve determinism by either:

- records.push(patientRecord);
+ records[patientRecord.rowNumber - 1] = patientRecord;

or sort afterwards.

---

60-75: Consider streaming to avoid loading all patient records into memory

For large imports every record is held twice (raw string + parsed object) before being written, which can exhaust Lambda memory.
Mapping each record directly to a ResultEntry and streaming storeResults chunk-by-chunk would keep memory constant.

---

90-93: Wrap JSON parse errors with context

If a single record file is malformed, the entire job fails without indicating which key was bad. Catch here, add the key to the error message, and rethrow.

packages/core/src/command/patient-import/steps/result/patient-import-result-local.ts (1)

26-28: Swallowing the command error hides failures from callers

While mimicking async cloud behaviour, any local invoker now has no way to know the job failed. Consider returning a boolean or logging the command’s updated status so upstream tests can assert success/failure.

packages/core/src/command/patient-import/csv/convert-patient.ts (2)

45-47: Redundant “missing first/last name” checks

normalizeName already throws for empty values, so the extra if (!firstName) / if (!lastName) blocks never fire. Remove them to simplify control flow.

Also applies to: 53-55

---

76-81: Duplicate address presence validation

You push an "Missing address" error at lines 78-81 and immediately repeat a generic check later (100-103). Drop one to avoid confusing, duplicated error messages.

Also applies to: 100-103

packages/core/src/command/patient-import/steps/parse/patient-import-parse-command.ts (2)

53-53: Drop the useless ternary in partition

p => p.status !== "failed" is already a boolean – the ? true : false adds noise without value.

🧰 Tools

🪛 Biome (1.9.4)

[error] 53-53: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

---

71-74: Promise.allSettled + internal try/catch is redundant

Because every mapped promise already catches its own error and never rejects, the outer Promise.allSettled provides no extra value.
A plain Promise.all (or even for … of with awaits) will be cheaper and a bit clearer.

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

676-710: Consider renaming the stack parameter to scope for clarity.

The parameter is used as the scope for the CDK construct, which is the standard pattern. Renaming it to scope would better align with CDK conventions and improve code readability.

-  private setupConsolidatedSearchConstruct(ownProps: {
-    stack: Stack;
+  private setupConsolidatedSearchConstruct(ownProps: {
+    scope: Construct;
     vpc: ec2.IVpc;
     config: EnvConfig;
     lambdaLayers: LambdaLayers;
     secrets: Secrets;
     medicalDocumentsBucket: s3.Bucket;
     featureFlagsTable: dynamodb.Table;
     openSearch: OpenSearchConfigForLambdas;
   }): ConsolidatedSearchConstruct {
     const {
-      stack,
+      scope,
       vpc,
       config,
       lambdaLayers,
       secrets,
       medicalDocumentsBucket,
       featureFlagsTable,
       openSearch,
     } = ownProps;
     const consolidatedSearchConstruct = new ConsolidatedSearchConstruct(
-      stack,
+      scope,
       "ConsolidatedSearchConstruct",
       {
         config,
         vpc,
         lambdaLayers,
         secrets,
         medicalDocumentsBucket,
         featureFlagsTable,
         openSearch,
       }
     );
     return consolidatedSearchConstruct;
   }

Also update the call site on line 176:

-      stack: this,
+      scope: this,

And remove the Stack import on line 1 since it's no longer needed (you can import Construct from 'constructs' if needed for the type).

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

Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.

Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.

packages/shared/src/common/__tests__/title-case.test.ts (1)

22-26: Nice addition – covers the new edge case
Good to see the apostrophe scenario exercised. Consider adding a variant with leading/trailing whitespace (e.g. " o'neal "), but that’s optional.

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

packages/shared/src/domain/patient/patient-import/create.ts (1)

33-36: Changing these defaults is a silent breaking change – audit call-sites & update docs/tests.

rerunPdOnNewDemographics and triggerConsolidated now default to true. Any existing caller that relied on the previous false default will suddenly trigger extra processing (PD re-runs, consolidated search) unless they explicitly pass false.

Please:

1. Confirm that every caller really wants this new behaviour; otherwise make them pass true explicitly and keep the default false.
2. Update unit/integration tests that assert default flag values.
3. Mention the new default in public docs / changelog to avoid surprises in downstream services.

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