feat(core): FHIR converter for Surescripts response files #4033
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
## Walkthrough
This update introduces a comprehensive Surescripts-to-FHIR conversion pipeline. It adds new modules and handlers for parsing, validating, and converting Surescripts response and verification files into FHIR bundles, including CLI tools and Lambda handlers for batch and patient response conversion. The SFTP client and action framework are extended with a "sync" method and action, and S3-based replica management is refactored. The infrastructure stack is updated to deploy new Lambda functions and environment variables, and several new mapping utilities are added for Surescripts code translations.
## Changes
| Files/Paths | Change Summary |
|-----------------------------------------------------------------------------|---------------|
| `.../sftp/client.ts`, `.../sftp-action/sftp-action-direct.ts`, `.../sftp-action/sftp-action.ts` | Added SFTP sync method/action, JSDoc, and result typing. |
| `.../surescripts/client.ts` | Refactored replica initialization, improved logging, updated imports. |
| `.../surescripts/replica.ts` | Added S3-based SurescriptsReplica for raw file retrieval and decompression. |
| `.../surescripts/file/file-generator.ts` | Removed file parsing logic; now only generates rows. |
| `.../surescripts/file/file-parser.ts` | New: Implements schema-driven parsing/validation for Surescripts files. |
| `.../surescripts/file/file-names.ts` | Added conversion bundle filename generator. |
| `.../surescripts/schema/response.ts`, `.../schema/verification.ts`, `.../schema/shared.ts` | Refactored schemas/types/validators, standardized naming, updated structure to wrap row data with source arrays. |
| `.../surescripts/types.ts` | Added SurescriptsConversionBundle interface. |
| `.../surescripts/fhir-converter.ts` | New: Converts Surescripts files to FHIR bundles, uploads bundles. |
| `.../surescripts/fhir/bundle.ts`, `.../fhir/bundle-entry.ts`, `.../fhir/condition.ts`, `.../fhir/coverage.ts`, `.../fhir/medication.ts`, `.../fhir/medication-dispense.ts`, `.../fhir/medication-request.ts`, `.../fhir/patient.ts`, `.../fhir/pharmacy.ts`, `.../fhir/prescriber.ts`, `.../fhir/provenance.ts`, `.../fhir/source-document.ts`, `.../fhir/shared.ts`, `.../fhir/constants.ts` | New: FHIR resource construction, deduplication, and mapping utilities. |
| `.../command/convert-batch-response/convert-batch-response-cloud.ts`, `.../convert-batch-response-direct.ts`, `.../convert-batch-response-factory.ts`, `.../convert-batch-response.ts` | New: Batch response conversion handlers (Lambda & direct), interface, and factory. |
| `.../command/convert-patient-response/convert-patient-response-cloud.ts`, `.../convert-patient-response-direct.ts`, `.../convert-patient-response-factory.ts`, `.../convert-patient-response.ts` | New: Patient response conversion handlers (Lambda & direct), interface, and factory. |
| `.../command/receive-response/receive-response-direct.ts` | Integrated batch response conversion into receive handler. |
| `.../surescripts/__tests__/file-names.test.ts`, `.../message.test.ts` | Updated imports for new file structure. |
| `.../surescripts/__tests__/flat-file-response.test.ts`, `.../verification-response.test.ts` | Updated to use new file parsing functions and access paths. |
| `.../util/config.ts` | Added Lambda/bucket config getters, removed old bundle bucket getter. |
| `.../infra/lib/api-stack.ts`, `.../api-stack/api-service.ts` | Updated Lambda env var assignment, removed unused API URL setter. |
| `.../infra/lib/surescripts/surescripts-stack.ts`, `.../surescripts/types.ts`| Added convertPatientResponse/convertBatchResponse Lambdas, refactored settings. |
| `.../lambdas/src/surescripts/convert-batch-response.ts`, `.../convert-patient-response.ts`, `.../shared.ts` | New Lambda handlers for patient/batch conversion, replica factory. |
| `.../shared/src/interface/external/surescripts/code-list-qualifier.ts`, `.../dea-schedule.ts`, `.../ncpdp.ts`, `.../payment-code.ts`, `.../plan-code.ts` | New mapping utilities for Surescripts code translation. |
| `.../utils/src/surescripts/analysis.ts`, `.../convert-response.ts`, `.../index.ts`, `.../sftp-action.ts` | New CLI tools for analysis, response conversion, SFTP sync, and command registration. |
## Sequence Diagram(s)
### Surescripts Batch Response Conversion (Direct Handler)
```mermaid
sequenceDiagram
participant CLI/Lambda
participant SurescriptsReplica
participant ConvertBatchResponseHandlerDirect
participant FHIRConverter
participant S3
CLI/Lambda->>SurescriptsReplica: getRawResponseFile(transmissionId, populationId)
SurescriptsReplica-->>CLI/Lambda: Buffer (raw response)
CLI/Lambda->>ConvertBatchResponseHandlerDirect: convertBatchResponse({cxId, transmissionId, populationId})
ConvertBatchResponseHandlerDirect->>FHIRConverter: convertBatchResponseToFhirBundles(rawResponse)
FHIRConverter-->>ConvertBatchResponseHandlerDirect: [ConversionBundle]
loop For each bundle
ConvertBatchResponseHandlerDirect->>FHIRConverter: uploadConversionBundle(bundle, cxId, patientId)
FHIRConverter->>S3: putObject(bundle)
S3-->>FHIRConverter: Success
end
ConvertBatchResponseHandlerDirect-->>CLI/Lambda: [ConversionBundle]SFTP Sync ActionsequenceDiagram
participant CLI
participant SftpClient
participant SftpReplica
participant SFTPServer
CLI->>SftpClient: sync(remotePath)
SftpClient->>SftpReplica: getFileList(replicaDir)
SftpClient->>SFTPServer: list(remotePath)
SFTPServer-->>SftpClient: [remoteFiles]
SftpClient-->>SftpReplica: [replicaFiles]
SftpClient->>SFTPServer: read(missingFile)
SFTPServer-->>SftpClient: fileContent
SftpClient->>SftpReplica: write(missingFile, fileContent)
SftpClient-->>CLI: [syncedFiles]
Possibly related PRs
|
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#2995 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
…ility ID metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
thomasyopes
reviewed
Jun 17, 2025
thomasyopes
reviewed
Jun 17, 2025
thomasyopes
reviewed
Jun 17, 2025
thomasyopes
reviewed
Jun 17, 2025
thomasyopes
reviewed
Jun 17, 2025
thomasyopes
reviewed
Jun 17, 2025
packages/core/src/external/surescripts/fhir/medication-request.ts
Outdated
Show resolved
Hide resolved
thomasyopes
reviewed
Jun 17, 2025
thomasyopes
reviewed
Jun 17, 2025
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/external/surescripts/fhir-converter.ts (3)
24-24: Avoid duplicate array creation.The
patientIdsarray is already available from line 21. Reuse it instead of creating a new array.- patientIds: Array.from(patientIdDetails.keys()).join(", "), + patientIds: patientIds.join(", "),
29-29: Remove unnecessary check.Since we verify
patientIds.length > 1above and the array has exactly one element at this point,patientIds[0]cannot be undefined.- const patientId = patientIds[0]; - if (!patientId) return undefined; + const patientId = patientIds[0]!;
104-104: Remove unnecessary optional chaining.Since we ensure the array exists on line 102, the optional chaining is not needed.
- patientIdDetails.get(patientId)?.push(detail); + patientIdDetails.get(patientId)!.push(detail);packages/core/src/external/surescripts/command/convert-patient-response/convert-patient-response-cloud.ts (1)
34-34: Consider adding runtime type validation.Direct type casting can hide runtime type errors. Consider validating the parsed JSON matches the expected
SurescriptsConversionBundletype.Add a type guard or validation function to ensure the parsed payload conforms to the expected type structure before returning it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/core/src/external/surescripts/command/convert-batch-response/convert-batch-response-direct.ts(1 hunks)packages/core/src/external/surescripts/command/convert-patient-response/convert-patient-response-cloud.ts(1 hunks)packages/core/src/external/surescripts/command/convert-patient-response/convert-patient-response-direct.ts(1 hunks)packages/core/src/external/surescripts/fhir-converter.ts(1 hunks)packages/core/src/external/surescripts/fhir/bundle-entry.ts(1 hunks)packages/core/src/external/surescripts/fhir/bundle.ts(1 hunks)packages/core/src/external/surescripts/fhir/condition.ts(1 hunks)packages/core/src/external/surescripts/fhir/coverage.ts(1 hunks)packages/core/src/external/surescripts/fhir/medication-request.ts(1 hunks)packages/core/src/external/surescripts/fhir/medication.ts(1 hunks)packages/core/src/external/surescripts/fhir/patient.ts(1 hunks)packages/core/src/external/surescripts/fhir/pharmacy.ts(1 hunks)packages/core/src/external/surescripts/fhir/prescriber.ts(1 hunks)packages/core/src/external/surescripts/file/file-names.ts(1 hunks)packages/core/src/external/surescripts/file/file-parser.ts(1 hunks)packages/lambdas/src/surescripts/convert-batch-response.ts(1 hunks)packages/lambdas/src/surescripts/convert-patient-response.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/lambdas/src/surescripts/convert-batch-response.ts
- packages/lambdas/src/surescripts/convert-patient-response.ts
- packages/core/src/external/surescripts/fhir/coverage.ts
- packages/core/src/external/surescripts/fhir/bundle-entry.ts
- packages/core/src/external/surescripts/fhir/bundle.ts
- packages/core/src/external/surescripts/fhir/medication-request.ts
- packages/core/src/external/surescripts/fhir/condition.ts
- packages/core/src/external/surescripts/fhir/patient.ts
- packages/core/src/external/surescripts/command/convert-patient-response/convert-patient-response-direct.ts
- packages/core/src/external/surescripts/fhir/pharmacy.ts
- packages/core/src/external/surescripts/fhir/medication.ts
- packages/core/src/external/surescripts/command/convert-batch-response/convert-batch-response-direct.ts
- packages/core/src/external/surescripts/fhir/prescriber.ts
- packages/core/src/external/surescripts/file/file-parser.ts
- packages/core/src/external/surescripts/file/file-names.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/core/src/external/surescripts/fhir-converter.tspackages/core/src/external/surescripts/command/convert-patient-response/convert-patient-response-cloud.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/core/src/external/surescripts/fhir-converter.ts (1)
59-93: LGTM! Good use of network retries and parallel uploads.The implementation properly handles network retries and efficiently uploads to both keys in parallel.
| if (!details || details.length < 1) return undefined; | ||
|
|
||
| const bundle = await convertIncomingDataToFhirBundle(patientId, details); | ||
| await hydrateFhir(bundle, console.log); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use out().log instead of console.log.
According to the coding guidelines, avoid using console.log in packages other than utils, infra and shared. Use out().log instead.
-import { executeWithNetworkRetries, MetriportError } from "@metriport/shared";
+import { executeWithNetworkRetries, MetriportError, out } from "@metriport/shared";Then update the logging calls:
- await hydrateFhir(bundle, console.log);
+ await hydrateFhir(bundle, out(console.log).log);Also applies to: 50-50
🤖 Prompt for AI Agents
In packages/core/src/external/surescripts/fhir-converter.ts at lines 34 and 50,
replace all instances of console.log with out().log to comply with the coding
guidelines. Import or ensure the out() function is available in the file, then
update the hydrateFhir calls and any other logging calls at these lines to use
out().log instead of console.log.
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
thomasyopes
reviewed
Jun 17, 2025
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/core/src/external/surescripts/fhir/constants.ts (1)
1-16: Constant-name casing still violates the repo guideline
Same concern raised previously: constants should becamelCase, notSCREAMING_SNAKE_CASE.
Re-raising only for completeness.
🧹 Nitpick comments (7)
packages/core/src/external/surescripts/fhir/constants.ts (1)
1-16: Mixed_URLvs_SYSTEMsuffixes introduce cognitive overheadThe file exports both
*_URLand*_SYSTEMconstants, e.g.DEA_SCHEDULE_URL(line 2) versusDEA_SCHEDULE_SYSTEM(line 12).
While the underlying URIs differ (sidvsCodeSystem), the suffix choice (URLvsSYSTEM) does not consistently map to that distinction across all constants (e.g.NPI_URLis also asid).Recommend adopting one naming convention, e.g. always
*Systemfor HL7 CodeSystem URIs and*IdentifierSystemor similar forsididentifiers, and documenting it in a short JSDoc at the top of the file.packages/core/src/external/surescripts/fhir/medication-dispense.ts (1)
64-79: Performer lacksfunctionelementFHIR allows (and many analytics workflows expect) a
performer[x].functionto indicate the role (e.g. “authorizing-prescriber”).
Consider adding:function: { coding: [ { system: "http://terminology.hl7.org/CodeSystem/medicationdispense-performer-function", code: "authorizing", display: "Authorizing prescriber" } ] }packages/utils/src/surescripts/convert-customer-response.ts (1)
18-24: Flagging param naming clarity
--csv-dataactually expects a filesystem path, not raw CSV text. Rename to--csv-path(and variablecsvPath) to avoid misuse.packages/utils/src/surescripts/shared.ts (1)
16-24: CSV column names are hard-coded
patient_idandtransmission_idare assumed. If the header casing or naming changes slightly the whole parse silently produces empty strings.Expose the column names as optional parameters with sensible defaults, or at minimum validate that the expected headers are present and throw a descriptive error otherwise.
packages/core/src/external/term-server/index.ts (1)
96-105: Crosswalk picks first mapping only
ConceptMap.group[0].element[0].target[0]may discard additional equally-valid targets or fail if the server returns multiple groups. Consider:
- iterating over all groups/elements to find the first
equivalentorequalmatch;- returning an array of
Codings if multiple targets exist.At minimum, document the assumption in a comment.
packages/core/src/external/surescripts/fhir/bundle.ts (1)
34-43: Hydrate medications in parallel for large bundles (optional)
awaitinside thefor…ofloop serialises NDC→RxNorm look-ups; batches with hundreds of meds will pay unnecessary latency. Collect promises andawait Promise.all(...)for better throughput.packages/core/src/external/surescripts/fhir-converter.ts (1)
48-56: Convert patients in parallel to cut batch latencyEach
convertIncomingDataToFhirBundlecall is independent. Building bundles sequentially can be several-minute latency for large batches.- for (const [patientId, details] of patientIdDetails.entries()) { - if (!details || details.length < 1) continue; - const bundle = await convertIncomingDataToFhirBundle(cxId, patientId, details); - conversionBundles.push({ cxId, patientId, bundle }); - } + const bundles = await Promise.all( + Array.from(patientIdDetails.entries()) + .filter(([, d]) => d && d.length > 0) + .map(async ([pid, det]) => ({ + cxId, + patientId: pid, + bundle: await convertIncomingDataToFhirBundle(cxId, pid, det), + })), + ); + conversionBundles.push(...bundles);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/core/src/external/surescripts/command/convert-batch-response/convert-batch-response-direct.ts(1 hunks)packages/core/src/external/surescripts/command/convert-patient-response/convert-patient-response-direct.ts(1 hunks)packages/core/src/external/surescripts/fhir-converter.ts(1 hunks)packages/core/src/external/surescripts/fhir/bundle.ts(1 hunks)packages/core/src/external/surescripts/fhir/condition.ts(1 hunks)packages/core/src/external/surescripts/fhir/constants.ts(1 hunks)packages/core/src/external/surescripts/fhir/medication-dispense.ts(1 hunks)packages/core/src/external/surescripts/fhir/medication-request.ts(1 hunks)packages/core/src/external/surescripts/fhir/medication.ts(1 hunks)packages/core/src/external/surescripts/fhir/pharmacy.ts(1 hunks)packages/core/src/external/surescripts/fhir/prescriber.ts(1 hunks)packages/core/src/external/surescripts/fhir/provenance.ts(1 hunks)packages/core/src/external/surescripts/fhir/shared.ts(1 hunks)packages/core/src/external/surescripts/file/file-names.ts(1 hunks)packages/core/src/external/surescripts/replica.ts(1 hunks)packages/core/src/external/surescripts/types.ts(3 hunks)packages/core/src/external/term-server/index.ts(4 hunks)packages/utils/src/surescripts/analysis.ts(1 hunks)packages/utils/src/surescripts/analyze-responses.ts(1 hunks)packages/utils/src/surescripts/batch-analysis.ts(1 hunks)packages/utils/src/surescripts/convert-customer-response.ts(1 hunks)packages/utils/src/surescripts/convert-response.ts(1 hunks)packages/utils/src/surescripts/index.ts(2 hunks)packages/utils/src/surescripts/shared.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/external/surescripts/fhir/prescriber.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/utils/src/surescripts/index.ts
- packages/utils/src/surescripts/convert-response.ts
- packages/core/src/external/surescripts/types.ts
- packages/core/src/external/surescripts/fhir/medication-request.ts
- packages/core/src/external/surescripts/fhir/pharmacy.ts
- packages/core/src/external/surescripts/command/convert-batch-response/convert-batch-response-direct.ts
- packages/core/src/external/surescripts/fhir/condition.ts
- packages/core/src/external/surescripts/command/convert-patient-response/convert-patient-response-direct.ts
- packages/core/src/external/surescripts/fhir/provenance.ts
- packages/utils/src/surescripts/batch-analysis.ts
- packages/core/src/external/surescripts/replica.ts
- packages/utils/src/surescripts/analysis.ts
- packages/core/src/external/surescripts/file/file-names.ts
- packages/utils/src/surescripts/analyze-responses.ts
- packages/core/src/external/surescripts/fhir/shared.ts
- packages/core/src/external/surescripts/fhir/medication.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/utils/src/surescripts/convert-customer-response.tspackages/utils/src/surescripts/shared.tspackages/core/src/external/surescripts/fhir/constants.tspackages/core/src/external/term-server/index.tspackages/core/src/external/surescripts/fhir/bundle.tspackages/core/src/external/surescripts/fhir-converter.tspackages/core/src/external/surescripts/fhir/medication-dispense.ts
🧠 Learnings (1)
packages/core/src/external/surescripts/fhir-converter.ts (2)
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/shared.ts:122-147
Timestamp: 2025-03-17T17:01:17.227Z
Learning: Avoid using console.log and console.error in packages other than utils, infra and shared, and try to use out().log instead according to Metriport's coding guidelines.
Learnt from: leite08
PR: metriport/metriport#3489
File: packages/api/src/routes/ehr/elation/appointment-webhook.ts:32-36
Timestamp: 2025-03-21T00:21:26.928Z
Learning: Use `out().log` instead of `console.log` for logging in packages other than utils, infra and shared.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/core/src/external/surescripts/fhir/medication-dispense.ts (1)
49-51:whenHandedOverderives fromdateWritten– verify mappingFHIR
MedicationDispense.whenHandedOvertypically corresponds to the dispense / pickup date, whereasdateWrittenis the prescription date.
Confirm that the Surescripts payload does not provide a more accurate dispense timestamp (e.g.,dateDispensed). If it does, prefer that field.packages/core/src/external/surescripts/fhir-converter.ts (1)
78-92: Retry wrapper may duplicate successful uploads
executeWithNetworkRetriesre-runs the entirePromise.all; if the first attempt only uploads one of the two objects before failing, retries will overwrite the successful object. S3putObjectis idempotent, but consider:
- Upload files separately within their own retry blocks, or
- Tag objects with
Content-MD5/If-None-Matchto avoid unnecessary overwrites.Not blocking, but worth noting for large bundles.
Comment on lines
+5
to
+10
| export const PLAN_NETWORK_PCN_SYSTEM = "http://terminology.hl7.org/CodeSystem/v2-0203"; | ||
| export const PLAN_NETWORK_BIN_SYSTEM = | ||
| "http://terminology.hl7.org/CodeSystem/NCPDPProviderIdentificationNumber"; | ||
| export const NCPDP_PROVIDER_ID_SYSTEM = | ||
| "http://terminology.hl7.org/CodeSystem/NCPDPProviderIdentificationNumber"; | ||
| export const COVERAGE_TYPE_SYSTEM = "http://terminology.hl7.org/CodeSystem/coverage-type"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate constants & ambiguous intent
PLAN_NETWORK_BIN_SYSTEM and NCPDP_PROVIDER_ID_SYSTEM resolve to the same URI.
Unless there is a functional distinction between BIN vs Provider ID that the URI alone cannot capture, this duplication risks:
- future drift when only one of the two is updated;
- confusion for downstream code authors choosing “the right one”.
Consider consolidating to a single constant (or an alias) and documenting its intended semantic scope.
🤖 Prompt for AI Agents
In packages/core/src/external/surescripts/fhir/constants.ts between lines 5 and
10, the constants PLAN_NETWORK_BIN_SYSTEM and NCPDP_PROVIDER_ID_SYSTEM both have
the same URI value, causing duplication and potential confusion. To fix this,
consolidate these two constants into a single constant or create one as an alias
of the other, and add clear documentation explaining the intended semantic
difference or usage scope to avoid ambiguity for future maintainers.
Comment on lines
+81
to
+92
| function getDaysSupply(detail: ResponseDetail): MedicationDispense["daysSupply"] | undefined { | ||
| if (!detail.daysSupply) return undefined; | ||
|
|
||
| const value = parseInt(detail.daysSupply); | ||
| if (!Number.isFinite(value)) return undefined; | ||
|
|
||
| return { | ||
| value, | ||
| unit: "day", | ||
| system: UNIT_OF_MEASURE_URL, | ||
| code: "d", | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
parseInt without radix & false negatives for “0”
- Always pass radix
10to avoid octal quirks in some JS engines. if (!detail.daysSupply)treats"0"as falsy; a 0-day supply is uncommon but valid and will be silently skipped.
- if (!detail.daysSupply) return undefined;
+ if (detail.daysSupply == null) return undefined;
- const value = parseInt(detail.daysSupply);
+ const value = parseInt(detail.daysSupply, 10);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function getDaysSupply(detail: ResponseDetail): MedicationDispense["daysSupply"] | undefined { | |
| if (!detail.daysSupply) return undefined; | |
| const value = parseInt(detail.daysSupply); | |
| if (!Number.isFinite(value)) return undefined; | |
| return { | |
| value, | |
| unit: "day", | |
| system: UNIT_OF_MEASURE_URL, | |
| code: "d", | |
| }; | |
| function getDaysSupply(detail: ResponseDetail): MedicationDispense["daysSupply"] | undefined { | |
| if (detail.daysSupply == null) return undefined; | |
| const value = parseInt(detail.daysSupply, 10); | |
| if (!Number.isFinite(value)) return undefined; | |
| return { | |
| value, | |
| unit: "day", | |
| system: UNIT_OF_MEASURE_URL, | |
| code: "d", | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/external/surescripts/fhir/medication-dispense.ts around
lines 81 to 92, update the parseInt call to include radix 10 to avoid octal
interpretation issues. Also, change the initial falsy check on detail.daysSupply
to explicitly check for null or undefined so that a "0" string is correctly
processed as a valid value instead of being skipped.
Comment on lines
+34
to
+41
| const handler = new SurescriptsConvertPatientResponseHandlerDirect(new SurescriptsReplica()); | ||
| await handler.convertPatientResponse({ | ||
| cxId, | ||
| facilityId, | ||
| transmissionId, | ||
| populationId: patientId, | ||
| }); | ||
| convertedCount++; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Object churn inside tight loop
A new SurescriptsReplica and SurescriptsConvertPatientResponseHandlerDirect is instantiated for every patient. For large CSVs this is avoidable overhead and may leak resources.
Create them once outside the loop:
- const transmissions = await getTransmissionsFromCsv(cxId, csvData);
- let convertedCount = 0;
- for (const { patientId, transmissionId } of transmissions) {
- const handler = new SurescriptsConvertPatientResponseHandlerDirect(new SurescriptsReplica());
+ const transmissions = await getTransmissionsFromCsv(cxId, csvData);
+ const replica = new SurescriptsReplica();
+ const handler = new SurescriptsConvertPatientResponseHandlerDirect(replica);
+ let convertedCount = 0;
+ for (const { patientId, transmissionId } of transmissions) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handler = new SurescriptsConvertPatientResponseHandlerDirect(new SurescriptsReplica()); | |
| await handler.convertPatientResponse({ | |
| cxId, | |
| facilityId, | |
| transmissionId, | |
| populationId: patientId, | |
| }); | |
| convertedCount++; | |
| const transmissions = await getTransmissionsFromCsv(cxId, csvData); | |
| const replica = new SurescriptsReplica(); | |
| const handler = new SurescriptsConvertPatientResponseHandlerDirect(replica); | |
| let convertedCount = 0; | |
| for (const { patientId, transmissionId } of transmissions) { | |
| await handler.convertPatientResponse({ | |
| cxId, | |
| facilityId, | |
| transmissionId, | |
| populationId: patientId, | |
| }); | |
| convertedCount++; | |
| } |
🤖 Prompt for AI Agents
In packages/utils/src/surescripts/convert-customer-response.ts around lines 34
to 41, the code creates a new SurescriptsReplica and
SurescriptsConvertPatientResponseHandlerDirect instance inside a loop for each
patient, causing unnecessary object churn and potential resource leaks. Move the
instantiation of SurescriptsReplica and
SurescriptsConvertPatientResponseHandlerDirect outside the loop so they are
created only once and reused for all patients.
Comment on lines
+83
to
+112
| export async function crosswalkNdcToRxNorm(ndcCode: string): Promise<Coding | undefined> { | ||
| const termServer = buildTermServerApi(); | ||
| if (!termServer) return undefined; | ||
|
|
||
| const params = buildFhirParametersForCrosswalkFromCoding( | ||
| { | ||
| system: NDC_URL, | ||
| code: ndcCode, | ||
| }, | ||
| RXNORM_URL | ||
| ); | ||
| if (!params) return undefined; | ||
|
|
||
| const result = await termServer.post(crosswalkUrl, params); | ||
|
|
||
| const data = result.data.response as ConceptMap; | ||
| const group = data.group?.[0]; | ||
| if (!group) return undefined; | ||
| const element = group.element?.[0]; | ||
| if (!element) return undefined; | ||
| const target = element.target?.[0]; | ||
| if (!target || !target.code) return undefined; | ||
|
|
||
| return { | ||
| system: RXNORM_URL, | ||
| code: target.code, | ||
| ...(target.display ? { display: target.display } : undefined), | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Network failures propagate raw Axios errors
crosswalkNdcToRxNorm currently throws any Axios error upstream. Wrap the call and re-throw as a domain-specific error (e.g., MetriportError) with the original Axios error as cause per guidelines, so callers can distinguish connectivity issues from “code not found”.
🤖 Prompt for AI Agents
In packages/core/src/external/term-server/index.ts around lines 83 to 112, the
function crosswalkNdcToRxNorm directly throws raw Axios errors on network
failures. To fix this, wrap the termServer.post call in a try-catch block, catch
any Axios errors, and re-throw them as a MetriportError including the original
Axios error as the cause. This will allow callers to differentiate between
connectivity issues and cases where the code is not found.
Comment on lines
+38
to
+44
| const ndcCode = medication.code?.coding?.find(coding => coding.system === NDC_URL); | ||
| if (!ndcCode || !ndcCode.code) continue; | ||
|
|
||
| const rxNormCode = await crosswalkNdcToRxNorm(ndcCode.code); | ||
| if (!rxNormCode) continue; | ||
| medication.code?.coding?.push(rxNormCode); | ||
| } |
There was a problem hiding this comment.
Ensure coding array exists before pushing
If medication.code.coding is undefined, the optional-chaining call silently does nothing, losing the RxNorm enrichment.
- medication.code?.coding?.push(rxNormCode);
+ if (!medication.code) medication.code = { coding: [] };
+ medication.code.coding = medication.code.coding ?? [];
+ medication.code.coding.push(rxNormCode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const ndcCode = medication.code?.coding?.find(coding => coding.system === NDC_URL); | |
| if (!ndcCode || !ndcCode.code) continue; | |
| const rxNormCode = await crosswalkNdcToRxNorm(ndcCode.code); | |
| if (!rxNormCode) continue; | |
| medication.code?.coding?.push(rxNormCode); | |
| } | |
| const ndcCode = medication.code?.coding?.find(coding => coding.system === NDC_URL); | |
| if (!ndcCode || !ndcCode.code) continue; | |
| const rxNormCode = await crosswalkNdcToRxNorm(ndcCode.code); | |
| if (!rxNormCode) continue; | |
| - medication.code?.coding?.push(rxNormCode); | |
| + if (!medication.code) medication.code = { coding: [] }; | |
| + medication.code.coding = medication.code.coding ?? []; | |
| + medication.code.coding.push(rxNormCode); | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/external/surescripts/fhir/bundle.ts around lines 38 to 44,
the code uses optional chaining when pushing rxNormCode to
medication.code.coding, which fails silently if coding is undefined. To fix
this, ensure medication.code.coding is initialized as an array before pushing by
checking if it exists and creating an empty array if not, then push rxNormCode
to it.
thomasyopes
approved these changes
Jun 18, 2025
| const sftpFileNames = await this.list(remotePath); | ||
| const replicaFileNamesWithPath = await this.replica.listFileNames(replicaDirectory); | ||
| const replicaDirectoryLength = replicaDirectory.length + 1; | ||
| const existingReplicaFileNames = new Set( |
| }) | ||
| .promise(); | ||
|
|
||
| if (!result.Payload) return undefined; |
There was a problem hiding this comment.
May not need this check if you're using failOnEmptyResponse true
| lambdaName: this.surescriptsConvertBatchResponseLambdaName, | ||
| failOnEmptyResponse: false, | ||
| }); | ||
| if (!resultPayload) return []; |
There was a problem hiding this comment.
May not need this check - should always be defined
| if (!entry.resource || entry.resource.resourceType !== "Medication") continue; | ||
| const medication = entry.resource as Medication; | ||
|
|
||
| const ndcCode = medication.code?.coding?.find(coding => coding.system === NDC_URL); |
There was a problem hiding this comment.
Look at EHR code for like getSnomedCodingForCondition -- there's a method we have that'll fetch first coding of a certain system. May be useful here.
| return undefined; | ||
| } | ||
| return await this.readFile(responseFile); | ||
| // const fileContent = await this.readFile(responseFile); |
1 task
5 tasks
This was referenced Jul 22, 2025
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
metriport/metriport-internal#2995
Issues:
Dependencies
Description
This introduces two new Lambda functions (single patient request and batch request) for converting Surescripts response files into FHIR bundles. These bundles are placed in the conversion bucket for pharmacy data sources, so they can be merged/deduplicated into the patient's consolidated bundle.
Testing
There is a Surescripts test facility and a Surescripts runbook in Notion which can be used to test this full flow E2E.
Release Plan
Execute FHIR conversion with Surescripts results where we know the patient ID and transmission ID (this is enough information to reliably extract the job results from S3).
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores