feat(ehr): elation contribution #4082
Conversation
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
WalkthroughThis change introduces comprehensive support for integrating the Elation EHR source into FHIR and CCDA workflows. It adds new handler functions, resource type mappings, and utility methods for CCDA and FHIR bundle management. The update also implements a flexible FHIR conversion handler system with cloud and direct modes, infrastructure enhancements for Lambda and S3 integration, and new utilities for file key/path generation and S3 operations. Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant ElationApi
participant S3
participant FHIRConverter
API->>ElationApi: getBundleByResourceType(params)
ElationApi->>ElationApi: getCcdaDocument()
ElationApi->>S3: Upload CCDA payload
ElationApi->>FHIRConverter: callConverter({payload, params})
FHIRConverter-->>ElationApi: FHIR Bundle
ElationApi->>API: Return validated FHIR Bundle
sequenceDiagram
participant HandlerFactory
participant ConversionFhirDirect
participant ConversionFhirCloud
participant Lambda
participant FHIRServer
HandlerFactory->>ConversionFhirDirect: if dev, instantiate
HandlerFactory->>ConversionFhirCloud: else, instantiate
ConversionFhirCloud->>Lambda: Invoke Lambda with payload
Lambda-->>ConversionFhirCloud: Return FHIR Bundle
ConversionFhirDirect->>FHIRServer: POST /api/convert/cda/ccd.hbs
FHIRServer-->>ConversionFhirDirect: Return FHIR Bundle
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
packages/shared/src/domain/converison-fhir.ts (1)
1-19: Fix filename typoThe filename has a typo:
converison-fhir.tsshould beconversion-fhir.ts. This typo will affect imports in other files and should be corrected.The enums and constant definitions are well-structured and follow the coding guidelines appropriately.
🧹 Nitpick comments (1)
packages/core/src/external/ehr/elation/command/get-resource-bundle-by-resource-id.ts (1)
19-21: Improve error message for better user experienceThe error message could be more descriptive to help users understand why this operation is not supported and what alternatives they have.
- throw new BadRequestError("useCachedBundle false is not supported"); + throw new BadRequestError("Non-cached bundle retrieval is not supported for Elation. Please use cached bundles only.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-jobs-across-ehrs.ts(1 hunks)packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts(1 hunks)packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts(1 hunks)packages/core/src/command/conversion-fhir/conversion-fhir-factory.ts(1 hunks)packages/core/src/command/conversion-fhir/conversion-fhir.ts(1 hunks)packages/core/src/command/conversion-fhir/shared.ts(1 hunks)packages/core/src/external/ehr/bundle/bundle-shared.ts(2 hunks)packages/core/src/external/ehr/canvas/index.ts(1 hunks)packages/core/src/external/ehr/command/get-bundle-by-resource-type.ts(2 hunks)packages/core/src/external/ehr/command/get-resource-bundle-by-resource-id.ts(2 hunks)packages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.ts(1 hunks)packages/core/src/external/ehr/elation/command/get-resource-bundle-by-resource-id.ts(1 hunks)packages/core/src/external/ehr/elation/index.ts(6 hunks)packages/core/src/external/ehr/job/create-resource-diff-bundles/shared.ts(1 hunks)packages/core/src/external/ehr/shared.ts(4 hunks)packages/core/src/util/config.ts(1 hunks)packages/shared/src/domain/converison-fhir.ts(1 hunks)packages/shared/src/interface/external/ehr/elation/ccda.ts(1 hunks)packages/shared/src/interface/external/ehr/elation/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column length is 100 chars Multi-line comments use /** */ Top-level comments go after the import (save pre-...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
packages/shared/src/interface/external/ehr/elation/index.tspackages/core/src/external/ehr/command/get-resource-bundle-by-resource-id.tspackages/core/src/external/ehr/bundle/bundle-shared.tspackages/core/src/external/ehr/command/get-bundle-by-resource-type.tspackages/core/src/external/ehr/job/create-resource-diff-bundles/shared.tspackages/core/src/util/config.tspackages/core/src/external/ehr/elation/command/get-resource-bundle-by-resource-id.tspackages/core/src/command/conversion-fhir/conversion-fhir-factory.tspackages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.tspackages/shared/src/interface/external/ehr/elation/ccda.tspackages/shared/src/domain/converison-fhir.tspackages/core/src/command/conversion-fhir/conversion-fhir.tspackages/core/src/command/conversion-fhir/conversion-fhir-cloud.tspackages/core/src/command/conversion-fhir/conversion-fhir-direct.tspackages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-jobs-across-ehrs.tspackages/core/src/external/ehr/canvas/index.tspackages/core/src/command/conversion-fhir/shared.tspackages/core/src/external/ehr/elation/index.tspackages/core/src/external/ehr/shared.ts
`**/*.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/shared/src/interface/external/ehr/elation/index.tspackages/core/src/external/ehr/command/get-resource-bundle-by-resource-id.tspackages/core/src/external/ehr/bundle/bundle-shared.tspackages/core/src/external/ehr/command/get-bundle-by-resource-type.tspackages/core/src/external/ehr/job/create-resource-diff-bundles/shared.tspackages/core/src/util/config.tspackages/core/src/external/ehr/elation/command/get-resource-bundle-by-resource-id.tspackages/core/src/command/conversion-fhir/conversion-fhir-factory.tspackages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.tspackages/shared/src/interface/external/ehr/elation/ccda.tspackages/shared/src/domain/converison-fhir.tspackages/core/src/command/conversion-fhir/conversion-fhir.tspackages/core/src/command/conversion-fhir/conversion-fhir-cloud.tspackages/core/src/command/conversion-fhir/conversion-fhir-direct.tspackages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-jobs-across-ehrs.tspackages/core/src/external/ehr/canvas/index.tspackages/core/src/command/conversion-fhir/shared.tspackages/core/src/external/ehr/elation/index.tspackages/core/src/external/ehr/shared.ts
🧠 Learnings (4)
packages/core/src/external/ehr/command/get-bundle-by-resource-type.ts (1)
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
packages/core/src/external/ehr/elation/command/get-resource-bundle-by-resource-id.ts (1)
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
packages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.ts (1)
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
packages/core/src/external/ehr/elation/index.ts (1)
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
🔇 Additional comments (17)
packages/core/src/util/config.ts (1)
318-325: LGTM! Clean configuration method additions.The new methods follow the established patterns in the
Configclass, use appropriate error handling withgetEnvVarOrFail(), and maintain consistent naming conventions.packages/core/src/external/ehr/canvas/index.ts (1)
36-39: LGTM! Improved import specificity.The import path refinement from
@metriport/sharedto@metriport/shared/interface/external/ehr/fhir-resourceenhances modularity and aligns with stricter type definitions.packages/shared/src/interface/external/ehr/elation/index.ts (1)
8-8: LGTM! Consistent export pattern.The CCDA module export follows the established pattern in the file and properly extends the public interface.
packages/core/src/external/ehr/bundle/bundle-shared.ts (2)
6-6: LGTM! Consistent EHR module import.The import follows the established pattern for other EHR sources and maintains consistency across the codebase.
64-64: LGTM! Proper EHR source integration.The Elation conditional branch follows the exact same pattern as Canvas and Athena implementations, ensuring consistent behavior across all EHR sources.
packages/core/src/external/ehr/command/get-resource-bundle-by-resource-id.ts (2)
6-6: LGTM! Consistent handler import pattern.The import follows the established naming convention and pattern used for other EHR source handlers.
45-45: LGTM! Proper handler registration.The Elation handler assignment replaces the previous
undefinedvalue and follows the same pattern as Canvas and Athena implementations, enabling resource bundle retrieval for Elation.packages/core/src/external/ehr/command/get-bundle-by-resource-type.ts (2)
6-6: LGTM: Import follows established pattern.The import statement correctly follows the same naming convention as existing EHR sources (Canvas and Athena).
36-36: LGTM: Consistent EHR source mapping.The addition of Elation to the
ehrGetBundleByResourceTypeMapcorrectly follows the established pattern and maintains consistency with other EHR sources.packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-jobs-across-ehrs.ts (1)
48-54: LGTM: Consistent job orchestration for Elation.The implementation correctly follows the established pattern for Canvas (without practiceId) and includes proper error handling with
processAsyncError. The conditional branch maintains consistency with existing EHR source handling.packages/core/src/external/ehr/job/create-resource-diff-bundles/shared.ts (1)
19-23: LGTM: Proper addition to supported EHR sources.The addition of
EhrSources.elationto the array is consistent with the existing pattern and maintains proper type safety with the const assertion.packages/shared/src/interface/external/ehr/elation/ccda.ts (1)
1-7: LGTM: Well-structured Zod schema for CCDA validation.The schema definition follows best practices:
z.coerce.string()for theidfield appropriately handles potential numeric inputs- Clear naming convention for the
base64_ccdafield- Proper type inference with TypeScript
- Consistent with Zod validation patterns
packages/core/src/command/conversion-fhir/conversion-fhir-factory.ts (1)
6-13: LGTM: Clean factory pattern implementation.The factory function follows good design principles:
- Environment-based handler selection is appropriate
- Clean separation between development and production configurations
- Proper use of configuration methods
- Follows TypeScript and coding guidelines correctly
packages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.ts (1)
6-39: Consider adding error handling for external service callsThe function lacks error handling for potential failures from the Elation client or FHIR conversion handler. Consider wrapping the external calls in try-catch blocks to provide more meaningful error messages to callers.
export async function getBundleByResourceType( params: GetBundleByResourceTypeClientRequest ): Promise<Bundle> { + try { const { tokenId, cxId, practiceId, metriportPatientId, ehrPatientId, resourceType, useCachedBundle, } = params; const client = await createElationHealthClient({ cxId, practiceId, ...(tokenId && { tokenId }), }); const handler = buildConversionFhirHandler(); const bundle = await client.getBundleByResourceType({ cxId, metriportPatientId, elationPatientId: ehrPatientId, resourceType, fhirConverter: async (payload: string) => { return await handler.convertToFhir({ cxId, patientId: metriportPatientId, rawData: payload, }); }, useCachedBundle, }); return bundle; + } catch (error) { + throw new Error(`Failed to get bundle by resource type for Elation: ${error.message}`, { + cause: error, + }); + } }⛔ Skipped due to learnings
Learnt from: thomasyopes PR: metriport/metriport#3882 File: packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:0-0 Timestamp: 2025-05-28T19:20:47.442Z Learning: In packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.Learnt from: thomasyopes PR: metriport/metriport#3882 File: packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:27-49 Timestamp: 2025-05-28T19:22:09.281Z Learning: In packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.Learnt from: thomasyopes PR: metriport/metriport#3788 File: packages/api/src/external/ehr/shared/utils/bundle.ts:83-93 Timestamp: 2025-05-08T19:41:36.533Z Learning: In the Metriport codebase, the team prefers to let errors bubble up naturally in some cases rather than adding explicit error handling at every layer, as demonstrated in the refreshEhrBundle function in the bundle.ts file.Learnt from: thomasyopes PR: metriport/metriport#3608 File: packages/core/src/external/ehr/canvas/index.ts:451-469 Timestamp: 2025-04-21T17:07:30.574Z Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.packages/core/src/command/conversion-fhir/conversion-fhir.ts (1)
4-19: Well-designed type definitions and interfaceThe
ConversionFhirRequesttype andConversionFhirHandlerinterface are well-structured with appropriate optional fields and clear method signatures. They follow TypeScript best practices and maintain good separation of concerns.packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts (1)
11-16: Remove redundant assignment in constructor.The assignment
this.lambdaClient = lambdaClient;on line 15 is redundant because the parameter is already assigned to the instance property through theprivatemodifier on line 13.constructor( private readonly nodejsFhirConvertLambdaName: string, lambdaClient: LambdaClient = makeLambdaClient(Config.getAWSRegion()) ) { - this.lambdaClient = lambdaClient; }Likely an incorrect or invalid review comment.
packages/core/src/external/ehr/shared.ts (1)
674-710: Well-structured bundle creation and partitioning logic.The function properly separates target and reference resources, validates them against schemas, and returns a clear structure with both bundles.
packages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/ehr/elation/command/get-resource-bundle-by-resource-id.ts
Show resolved
Hide resolved
packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts
Outdated
Show resolved
Hide resolved
…metriport/metriport into eng-139-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/core/src/command/conversion-fhir/conversion-fhir.ts (1)
2-2: Fix import path to match corrected filename.The import references the misspelled filename. This should be corrected to match the fixed filename from the previous file.
-import { FHIRConverterCDATemplate } from "@metriport/shared/domain/converison-fhir"; +import { FHIRConverterCDATemplate } from "@metriport/shared/domain/conversion-fhir";packages/core/src/external/ehr/elation/index.ts (2)
341-360: Fix typo in parameter name.The parameter name has a typo: "elationPatinetId" should be "elationPatientId" (note the "n" and "t" are swapped).
async getBundleByResourceType({ cxId, metriportPatientId, - elationPatinetId, + elationPatientId, resourceType, - fhirConverterToEhrBundle, + fhirConverterToEhrBundle, useCachedBundle = true, }: { cxId: string; metriportPatientId: string; - elationPatinetId: string; + elationPatientId: string; resourceType: string; - fhirConverterToEhrBundle: (params: { + fhirConverterToEhrBundle: (params: { inputS3Key: string; inputS3BucketName: string; outputS3Key: string; outputS3BucketName: string; }) => Promise<EhrFhirResourceBundle>; useCachedBundle?: boolean; }): Promise<Bundle> {
366-410: Update all references to the corrected parameter name.After fixing the parameter name typo, update all references within the method.
- const xml = await this.getCcdaDocument({ cxId, patientId: elationPatinetId, resourceType }); + const xml = await this.getCcdaDocument({ cxId, patientId: elationPatientId, resourceType });- ehrPatientId: elationPatinetId, + ehrPatientId: elationPatientId,- ehrPatientId: elationPatinetId, + ehrPatientId: elationPatientId,
🧹 Nitpick comments (2)
packages/core/src/external/ehr/bundle/command/create-or-replace-ccda.ts (1)
13-25: Fix JSDoc documentation inconsistency.The JSDoc mentions an
s3FileNameparameter that doesn't exist in the function signature. Remove this line from the documentation.- * @param s3FileName - The S3 file name of the CCDA file.packages/core/src/command/conversion-fhir/shared.ts (1)
30-79: Fix typo in parameter name.There's a typo in the parameter name on line 34: "parmas" should be "params".
- convertToFhir: (payload: string, parmas: FhirConverterParams) => Promise<Bundle<Resource>>; + convertToFhir: (payload: string, params: FhirConverterParams) => Promise<Bundle<Resource>>;Otherwise, the function implementation is well-structured with proper logging, deduplication using Set, and appropriate async handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/core/src/command/conversion-fhir/conversion-fhir.ts(1 hunks)packages/core/src/command/conversion-fhir/shared.ts(1 hunks)packages/core/src/domain/conversion/filename.ts(1 hunks)packages/core/src/external/ehr/bundle/bundle-shared.ts(5 hunks)packages/core/src/external/ehr/bundle/command/create-or-replace-bundle.ts(1 hunks)packages/core/src/external/ehr/bundle/command/create-or-replace-ccda.ts(1 hunks)packages/core/src/external/ehr/bundle/command/fetch-bundle.ts(2 hunks)packages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.ts(1 hunks)packages/core/src/external/ehr/elation/index.ts(6 hunks)packages/core/src/util/config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/external/ehr/bundle/command/create-or-replace-bundle.ts
- packages/core/src/external/ehr/bundle/command/fetch-bundle.ts
- packages/core/src/domain/conversion/filename.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/external/ehr/bundle/bundle-shared.ts
- packages/core/src/util/config.ts
- packages/core/src/external/ehr/elation/command/get-bundle-by-resource-type.ts
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column length is 100 chars Multi-line comments use /** */ Top-level comments go after the import (save pre-...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
packages/core/src/external/ehr/bundle/command/create-or-replace-ccda.tspackages/core/src/command/conversion-fhir/conversion-fhir.tspackages/core/src/command/conversion-fhir/shared.tspackages/core/src/external/ehr/elation/index.ts
`**/*.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/ehr/bundle/command/create-or-replace-ccda.tspackages/core/src/command/conversion-fhir/conversion-fhir.tspackages/core/src/command/conversion-fhir/shared.tspackages/core/src/external/ehr/elation/index.ts
🧠 Learnings (1)
packages/core/src/external/ehr/elation/index.ts (1)
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (13)
packages/core/src/external/ehr/bundle/command/create-or-replace-ccda.ts (4)
1-4: LGTM! Well-organized imports.The imports are properly structured and all appear to be used in the implementation.
6-11: LGTM! Well-defined type using proper TypeScript patterns.The
CreateOrReplaceCcdaParamstype correctly omits unnecessary properties from the base type and adds the required payload property.
26-38: LGTM! Well-designed function signature.The function signature properly uses destructuring, has appropriate default values, and returns a well-typed object.
39-74: LGTM! Solid implementation with proper error handling.The implementation correctly:
- Uses proper logging with context
- Implements network retries for resilience
- Sets appropriate content type for XML
- Provides detailed error context
- Follows the established patterns for S3 operations
packages/core/src/command/conversion-fhir/conversion-fhir.ts (2)
4-15: LGTM! Well-structured type definition.The
ConversionFhirRequesttype is well-designed with:
- Clear property names following camelCase convention
- Appropriate use of optional properties
- Proper separation of input/output S3 configurations
- Meaningful conversion control flags
17-21: LGTM! Clean interface design.The
ConversionFhirHandlerinterface follows good design principles with a single, focused method that has a clear contract.packages/core/src/command/conversion-fhir/shared.ts (3)
1-28: LGTM! Well-organized imports and constants.The imports are properly structured and the
LARGE_CHUNK_SIZE_IN_BYTESconstant follows the guideline of moving literals to constants. ThegetS3Utilshelper function provides good encapsulation.
119-155: LGTM! Well-implemented intermediate step saving.The function properly handles both string and Bundle types, uses appropriate error handling without re-throwing (since step saving is optional), and provides good logging for debugging purposes.
157-171: LGTM! Clean and focused output saving function.The function is appropriately simple and focused on its single responsibility of saving the final conversion result to the output bucket.
packages/core/src/external/ehr/elation/index.ts (4)
78-127: LGTM! Well-structured constants and type definitions.The new constants and type guards are well-implemented:
- Clear mapping between FHIR resource types and Elation CCDA sections
- Proper TypeScript type guards with runtime validation
- Good separation between main resources and reference resources
- Currently conservative approach with only AllergyIntolerance enabled
217-259: LGTM! Well-implemented CCDA document fetching.The
getCcdaDocumentmethod is properly implemented with:
- Input validation for supported resource types
- Proper URL parameter handling for section filtering
- Correct base64 decoding
- Integration with the CCDA storage utility
- Good error handling and logging
361-410: LGTM! Sound implementation logic once parameter typos are fixed.The method implementation follows good patterns:
- Proper resource type validation
- Correct FHIR conversion workflow
- Bundle partitioning for target vs reference resources
- Appropriate caching strategy
- Async reference bundle saving
The return type
Promise<Bundle>is correct per established patterns where this method can always return a bundle by fetching from the EHR if needed.
412-446: LGTM! Well-implemented resource bundle retrieval by ID.The
getResourceBundleByResourceIdmethod is correctly implemented:
- Validates both main and reference resource types
- Appropriately uses cache-only strategy with empty fetch function
- Proper error handling with meaningful context
- Correct usage pattern for retrieving specific resources by ID
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/api/src/external/cda/generate-ccd.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column length is 100 chars Multi-line comments use /** */ Top-level comments go after the import (save pre-...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
packages/api/src/external/cda/generate-ccd.ts
`**/*.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/api/src/external/cda/generate-ccd.ts
🪛 Biome (1.9.4)
packages/api/src/external/cda/generate-ccd.ts
[error] 55-55: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (1)
packages/api/src/external/cda/generate-ccd.ts (1)
23-23: LGTM!The import statement is correctly structured and follows the project's import organization conventions.
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
…metriport/metriport into eng-139-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
…metriport/metriport into eng-139-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
…metriport/metriport into eng-139-elation-contribution Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/command/conversion-fhir/convert-payload-to-fhir.ts (1)
70-73: Consider using a dedicated batch bundle builder function.Based on the past review comment, there's a preference to deprecate
buildBundleFromResourcesand create dedicated functions likebuildBatchBundlefor each bundle type, since each has its own logic (e.g., searchset bundles havetotal).Consider creating a dedicated
buildBatchBundlefunction instead:- const bundle = buildBundleFromResources({ - type: "batch", - resources: [...resources.values()], - }); + const bundle = buildBatchBundle([...resources.values()]);
🧹 Nitpick comments (3)
packages/core/src/command/conversion-fhir/convert-payload-to-fhir.ts (3)
101-105: Strengthen the nonXMLBody validation.The current string inclusion check for "nonXMLBody" might be fragile and could produce false positives if this text appears in legitimate XML content.
Consider a more robust check:
- if (payloadRaw.includes("nonXMLBody")) { + if (payloadRaw.includes("<nonXMLBody>")) { throw new BadRequestError("XML document is unstructured CDA with nonXMLBody", undefined, { ...additionalInfo, }); }
115-118: Address the B64 attachments TODO.The extraction of base64 PDF attachments is logged but not processed, leaving functionality incomplete.
This TODO comment indicates missing B64 attachment processing. The extracted attachments are currently ignored, which might result in incomplete FHIR conversion.
Would you like me to help implement the B64 attachment processing or open a new issue to track this requirement?
141-143: Address the TODO for optional parameters.The converter parameters
unusedSegmentsandinvalidAccessare hardcoded as "false" with a TODO indicating they should be optional.The hardcoded converter parameters suggest incomplete flexibility in the conversion process.
Would you like me to help implement optional parameter handling or open a new issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/command/conversion-fhir/convert-payload-to-fhir.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column length is 100 chars Multi-line comments use /** */ Top-level comments go after the import (save pre-...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
packages/core/src/command/conversion-fhir/convert-payload-to-fhir.ts
`**/*.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/command/conversion-fhir/convert-payload-to-fhir.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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 (3)
packages/core/src/command/conversion-fhir/convert-payload-to-fhir.ts (3)
149-194: Good error handling strategy for intermediate steps.The
saveConverterStepfunction correctly implements the error handling strategy mentioned in past reviews - throwing errors for result files but not for intermediate steps via thethrowErrorparameter.
32-82: Well-structured main conversion function.The main function follows good practices with proper error handling, resource deduplication using Set, chunk size monitoring, and comprehensive logging. The return type is well-defined and the function signature is clear.
50-63: Effective chunk size monitoring.The implementation correctly monitors chunk sizes and uses structured logging with capture.message for large chunks, providing good observability for performance issues.
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/command/conversion-fhir/utils.ts (3)
44-48: Consider more robust validation for unstructured CDA detection.The current string search for "nonXMLBody" could potentially produce false positives if this text appears in legitimate XML content or comments. Consider using XML parsing or more specific pattern matching to detect unstructured CDA documents.
- if (payloadRaw.includes("nonXMLBody")) { + if (payloadRaw.includes("<nonXMLBody>") || payloadRaw.includes("nonXMLBody")) {
84-86: Address the TODO comment for converter parameters.The hardcoded
unusedSegmentsandinvalidAccessparameters should be made configurable or removed if not needed. This reduces technical debt and improves maintainability.Would you like me to help generate a solution that makes these parameters optional or configurable based on the conversion requirements?
119-124: Optimize file upload for large payloads.Creating a Buffer from the entire result string could consume significant memory for large FHIR bundles. Consider using streaming uploads for better memory efficiency.
- file: Buffer.from(typeof result === "string" ? result : JSON.stringify(result), "utf8"), + file: typeof result === "string" ? result : JSON.stringify(result),Note: This assumes S3Utils.uploadFile can accept string input directly. If not, consider implementing streaming for large payloads.
packages/core/src/command/conversion-fhir/conversion-fhir.ts (2)
40-52: Optimize chunk size calculation and improve telemetry threshold.Using
new Blob([payload]).sizeto calculate chunk size creates an unnecessary object. Consider usingBuffer.byteLength()for better performance. Also, the 50MB threshold seems arbitrary - consider making it configurable.- const chunkSize = new Blob([payload]).size; + const chunkSize = Buffer.byteLength(payload, 'utf8');Consider adding a configuration constant:
-const LARGE_CHUNK_SIZE_IN_BYTES = 50_000_000; +const LARGE_CHUNK_SIZE_IN_BYTES = Config.getFhirConversionChunkSizeThreshold() ?? 50_000_000;
54-58: Enhance validation logic for conversion results.The current validation could be more explicit about what constitutes a valid conversion result. Consider adding more descriptive logging for skipped results.
- if (!conversionResult || !conversionResult.entry || conversionResult.entry.length < 1) + if (!conversionResult || !conversionResult.entry || conversionResult.entry.length < 1) { + log(`Skipping chunk ${index} - no valid conversion result or entries`); continue; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/command/conversion-fhir/conversion-fhir.ts(1 hunks)packages/core/src/command/conversion-fhir/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column length is 100 chars Multi-line comments use /** */ Top-level comments go after the import (save pre-...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
packages/core/src/command/conversion-fhir/utils.tspackages/core/src/command/conversion-fhir/conversion-fhir.ts
`**/*.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/command/conversion-fhir/utils.tspackages/core/src/command/conversion-fhir/conversion-fhir.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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 (3)
packages/core/src/command/conversion-fhir/utils.ts (1)
92-106: Well-designed function signature with good error handling options.The
saveConverterStepfunction provides flexible error handling with thethrowErrorparameter and comprehensive logging. The type union forresultparameter appropriately handles both string and Bundle types.packages/core/src/command/conversion-fhir/conversion-fhir.ts (2)
38-39: Good use of Set for resource deduplication.Using a Set to collect resources effectively prevents duplicates across chunks while maintaining good performance characteristics. This is a solid design choice for handling potentially overlapping conversion results.
26-31: Well-designed async method signature with comprehensive return type.The
convertToFhirmethod provides a clean interface that returns both the converted bundle and S3 storage details, which is essential for tracking conversion results. The return type clearly communicates what consumers can expect.
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Approving w/ this comment open so we can ship it. Hopefully we can at least mark the buildBundleFromResources function as deprecated.
Ref: ENG-139 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/command/conversion-fhir/utils.ts (1)
25-76: Well-structured payload processing with proper validation.The function correctly handles the conversion pipeline: S3 retrieval, validation, cleaning, and partitioning. The error handling is comprehensive with proper context information.
One minor observation: The TODO comment at line 57 indicates incomplete work for B64 attachments processing. Consider tracking this as a separate issue.
packages/core/src/command/conversion-fhir/conversion-fhir.ts (2)
29-82: Consider more accurate byte size calculation for chunk monitoring.The chunk size calculation using
new Blob([payload]).sizemay not accurately represent the UTF-8 byte size of the string. Consider usingBuffer.byteLength(payload, 'utf8')for more precise measurement.- const chunkSize = new Blob([payload]).size; + const chunkSize = Buffer.byteLength(payload, 'utf8');Otherwise, the conversion orchestration logic is well-implemented with proper resource deduplication using Set and comprehensive logging.
47-50: Address the TODO comment for converter parameters.The TODO comment at line 47 indicates that
unusedSegmentsandinvalidAccessparameters should be made optional. Consider tracking this as a separate issue to improve the API design.Do you want me to generate a solution that makes these parameters optional or open a new issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/command/conversion-fhir/conversion-fhir.ts(1 hunks)packages/core/src/command/conversion-fhir/utils.ts(1 hunks)packages/core/src/external/fhir/bundle/bundle.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column length is 100 chars Multi-line comments use /** */ Top-level comments go after the import (save pre-...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
packages/core/src/external/fhir/bundle/bundle.tspackages/core/src/command/conversion-fhir/utils.tspackages/core/src/command/conversion-fhir/conversion-fhir.ts
`**/*.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/fhir/bundle/bundle.tspackages/core/src/command/conversion-fhir/utils.tspackages/core/src/command/conversion-fhir/conversion-fhir.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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 (5)
packages/core/src/external/fhir/bundle/bundle.ts (2)
177-183: Well-implemented batch bundle builder function.The function correctly creates a batch-type FHIR bundle from resources, following the same pattern as
buildBundleFromResources. The implementation is consistent with the existing codebase style.
323-327: Function refactoring maintains correctness.The change from arrow function to function declaration preserves the original logic and behavior. This refactoring aligns with the coding guidelines preference to avoid creating arrow functions.
packages/core/src/command/conversion-fhir/utils.ts (1)
78-123: Robust file saving with appropriate error handling.The function properly handles S3 uploads with comprehensive error handling and optional error throwing. The use of
Buffer.from()for both string and object serialization is correct.packages/core/src/command/conversion-fhir/conversion-fhir.ts (2)
16-27: Well-defined types for the conversion workflow.The type definitions are clear and properly structured.
ConversionFhirRequestandConverterRequestprovide good separation of concerns between input parameters and converter-specific data.
84-84: Abstract method design follows good patterns.The abstract
callConvertermethod provides a clean interface for subclasses to implement environment-specific conversion logic while maintaining consistent input/output contracts.
Ref: ENG-139
Issues:
Dependencies
Description
Testing
Release Plan
Summary by CodeRabbit
New Features
Improvements
Chores