1040 Add queue after FHIR converter #3612
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
7127b16 to
5674294
Compare
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
cf5303a to
f4edae9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
packages/core/src/command/conversion-result/conversion-result-cloud.ts (1)
14-35: Enhance error handling to make the function more idempotentWhile the error handling is good, consider making the function more idempotent by adding a retry mechanism with exponential backoff for transient SQS errors.
async notifyApi(params: ConversionResult, logParam?: typeof console.log): Promise<void> { const { cxId, jobId } = params; const { log } = logParam ? { log: logParam } : out(`notifyApi.cloud - cxId ${cxId} jobId ${jobId}`); try { const payload = JSON.stringify(params); await this.sqsClient.sendMessageToQueue(this.conversionResultQueueUrl, payload); } catch (error) { + // Check if error is retriable (e.g., network issue, throttling) + const isRetriable = error instanceof Error && + (error.name === 'NetworkingError' || + error.name === 'ThrottlingException' || + error.name === 'ServiceUnavailable'); + const msg = `Failure while processing conversion result @ ConversionResult`; log(`${msg}. Cause: ${errorToString(error)}`); capture.error(msg, { extra: { cxId, jobId, context: "conversion-result-cloud.notifyApi", error, + isRetriable, }, }); + // Rethrow with additional context to potentially handle retries upstream + throw new Error(`${msg}: ${errorToString(error)}`, { cause: error }); - throw error; } }packages/api/src/external/hie/__tests__/tally-doc-query-progress.test.ts (1)
11-45: Consider adding tests for edge cases with zero or undefined valuesWhile the test coverage is good, consider adding tests for more edge cases such as:
- Input with zero successes and zero errors
- Input with undefined successful but defined errors
- Input with undefined errors but defined successful
These tests would ensure robust handling of all possible input scenarios.
packages/core/src/command/conversion-result/conversion-result-local.ts (4)
6-6: Use a constant with descriptive naming.The constant name
MAX_API_NOTIFICATION_ATTEMPTSis good, but consider moving it to a constants file if it might be reused elsewhere.
10-10: Extract URL path to a constant.Hard-coded URLs should be extracted to constants, preferably at the top of the file or in a dedicated constants file.
- private readonly docProgressURL = `/internal/docs/conversion-status`; + private static readonly DOC_PROGRESS_URL = `/internal/docs/conversion-status`; + private readonly docProgressURL = ConversionResultLocal.DOC_PROGRESS_URL;
16-26: Improve error handling fornotifyApimethod.The method properly uses
executeWithNetworkRetriesfor network retries, but doesn't provide specific error handling if the retries fail. Consider adding more detailed error logging to help with debugging.async notifyApi(params: ConversionResultWithCount, logParam?: typeof console.log): Promise<void> { const { cxId, patientId, jobId } = params; const { log } = logParam ? { log: logParam } : out(`notifyApi.local - cx ${cxId} patient ${patientId} job ${jobId}`); log(`Notifying API on ${this.docProgressURL} w/ ${JSON.stringify(params)}`); - await executeWithNetworkRetries(() => this.api.post(this.docProgressURL, null, { params }), { - retryOnTimeout: true, - maxAttempts: MAX_API_NOTIFICATION_ATTEMPTS, - }); + try { + await executeWithNetworkRetries(() => this.api.post(this.docProgressURL, null, { params }), { + retryOnTimeout: true, + maxAttempts: MAX_API_NOTIFICATION_ATTEMPTS, + }); + } catch (error) { + const msg = `Failed to notify API after ${MAX_API_NOTIFICATION_ATTEMPTS} attempts`; + log(`${msg}: ${errorToString(error)}`); + throw new MetriportError(msg, { cause: error }); + } }
18-20: Simplify conditional logic for log initialization.The conditional logic for log initialization can be simplified using nullish coalescing.
- const { log } = logParam - ? { log: logParam } - : out(`notifyApi.local - cx ${cxId} patient ${patientId} job ${jobId}`); + const { log } = { log: logParam ?? out(`notifyApi.local - cx ${cxId} patient ${patientId} job ${jobId}`).log };packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (1)
26-26: Consider using a more type-safe approach for groupBy key.The current implementation creates a string key by concatenating values with
|. This works but is not type-safe and could be error-prone.-const groupedByPatientId = groupBy(results, r => r.patientId + "|" + r.source + "|" + r.status); +// Define a type for the group key +type ConversionResultGroupKey = { + patientId: string; + source: string | undefined; + status: string; +}; + +// Create a function to generate a stable string key +const createGroupKey = (result: ConversionResult): string => { + return `${result.patientId}|${result.source ?? ''}|${result.status}`; +}; + +// Group results using the key function +const groupedByPatientId = groupBy(results, createGroupKey);packages/infra/lib/api-stack.ts (1)
578-579: Consider using optional chaining for concise code.The conditional access pattern here could be simplified using optional chaining for better readability.
- sandboxSeedDataBucket && - sandboxSeedDataBucket.grantReadWrite(apiService.taskDefinition.taskRole); + sandboxSeedDataBucket?.grantReadWrite(apiService.taskDefinition.taskRole);🧰 Tools
🪛 Biome (1.9.4)
[error] 578-579: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/api/src/command/medical/document/document-conversion-status.ts (2)
41-41: Use nullish coalescing for clarity.Instead of
countParam == undefined ? 1 : countParam;, consider using the more idiomatic nullish coalescing operator:- const count = countParam == undefined ? 1 : countParam; + const count = countParam ?? 1;
45-45: Ensure log content is safe and concise.Logging
count,details, andconvertResultis helpful for debugging. However, ifdetailscan contain sensitive or large data, consider masking or truncating to prevent potential PII leakage or oversized logs.packages/infra/lib/lambdas-nested-stack.ts (1)
454-520: Add newsetupConversionResultNotifierfunction.This function provides a well-structured pattern for queue creation and Lambda setup, including SQS event source configuration. Consider adding a dead-letter queue if there's a risk of repeated failures and ensuring partial batch handling meets your operational needs. Otherwise, this is a solid implementation for an event-driven architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/api/src/command/medical/document/__tests__/document-query_calculateAndUpdateDocQuery.test.ts(4 hunks)packages/api/src/command/medical/document/document-conversion-status.ts(4 hunks)packages/api/src/command/medical/document/document-query.ts(2 hunks)packages/api/src/domain/medical/conversion-progress.ts(2 hunks)packages/api/src/external/hie/__tests__/tally-doc-query-progress.test.ts(1 hunks)packages/api/src/external/hie/tally-doc-query-progress.ts(4 hunks)packages/api/src/routes/internal/medical/docs.ts(2 hunks)packages/core/src/command/conversion-result/__tests__/send-multiple-conversion-results.test.ts(1 hunks)packages/core/src/command/conversion-result/conversion-result-cloud.ts(1 hunks)packages/core/src/command/conversion-result/conversion-result-factory.ts(1 hunks)packages/core/src/command/conversion-result/conversion-result-local.ts(1 hunks)packages/core/src/command/conversion-result/send-multiple-conversion-results.ts(1 hunks)packages/core/src/command/conversion-result/types.ts(1 hunks)packages/infra/lib/api-stack.ts(2 hunks)packages/infra/lib/api-stack/fhir-converter-connector.ts(8 hunks)packages/infra/lib/lambdas-nested-stack.ts(5 hunks)packages/infra/lib/shared/sqs.ts(1 hunks)packages/lambdas/src/conversion-result-notifier.ts(1 hunks)packages/lambdas/src/shared/oss-api.ts(1 hunks)packages/lambdas/src/sqs-to-converter.ts(7 hunks)
🧰 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/api/src/routes/internal/medical/docs.tspackages/core/src/command/conversion-result/conversion-result-cloud.tspackages/core/src/command/conversion-result/conversion-result-factory.tspackages/lambdas/src/conversion-result-notifier.tspackages/api/src/external/hie/__tests__/tally-doc-query-progress.test.tspackages/lambdas/src/shared/oss-api.tspackages/core/src/command/conversion-result/__tests__/send-multiple-conversion-results.test.tspackages/lambdas/src/sqs-to-converter.tspackages/core/src/command/conversion-result/types.tspackages/infra/lib/shared/sqs.tspackages/api/src/domain/medical/conversion-progress.tspackages/api/src/command/medical/document/__tests__/document-query_calculateAndUpdateDocQuery.test.tspackages/infra/lib/api-stack.tspackages/api/src/external/hie/tally-doc-query-progress.tspackages/core/src/command/conversion-result/send-multiple-conversion-results.tspackages/infra/lib/api-stack/fhir-converter-connector.tspackages/infra/lib/lambdas-nested-stack.tspackages/api/src/command/medical/document/document-conversion-status.tspackages/api/src/command/medical/document/document-query.tspackages/core/src/command/conversion-result/conversion-result-local.ts
🧬 Code Definitions (11)
packages/api/src/routes/internal/medical/docs.ts (1)
packages/api/src/routes/util.ts (1)
getFrom(131-133)
packages/core/src/command/conversion-result/conversion-result-cloud.ts (1)
packages/core/src/command/conversion-result/types.ts (2)
ConversionResultHandler(15-17)ConversionResult(1-9)
packages/core/src/command/conversion-result/conversion-result-factory.ts (3)
packages/core/src/command/conversion-result/types.ts (1)
ConversionResultHandler(15-17)packages/core/src/command/conversion-result/conversion-result-local.ts (1)
ConversionResultLocal(8-27)packages/core/src/command/conversion-result/conversion-result-cloud.ts (1)
ConversionResultCloud(7-36)
packages/lambdas/src/conversion-result-notifier.ts (3)
packages/lambdas/src/sqs-to-converter.ts (1)
handler(101-337)packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (1)
sendConversionResults(15-64)packages/core/src/command/conversion-result/types.ts (1)
ConversionResult(1-9)
packages/api/src/external/hie/__tests__/tally-doc-query-progress.test.ts (1)
packages/api/src/external/hie/tally-doc-query-progress.ts (2)
DynamicProgress(14-14)setHIETallyCount(85-125)
packages/core/src/command/conversion-result/__tests__/send-multiple-conversion-results.test.ts (3)
packages/core/src/command/conversion-result/types.ts (1)
ConversionResult(1-9)packages/core/src/command/conversion-result/conversion-result-local.ts (1)
ConversionResultLocal(8-27)packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (1)
sendConversionResults(15-64)
packages/lambdas/src/sqs-to-converter.ts (1)
packages/core/src/command/conversion-result/conversion-result-factory.ts (1)
buildConversionResultHandler(7-15)
packages/api/src/command/medical/document/__tests__/document-query_calculateAndUpdateDocQuery.test.ts (1)
packages/api/src/domain/medical/conversion-progress.ts (2)
ConvertResult(8-8)calculateConversionProgress(10-26)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (2)
packages/core/src/command/conversion-result/types.ts (2)
ConversionResult(1-9)ConversionResultWithCount(11-13)packages/core/src/command/conversion-result/conversion-result-local.ts (1)
ConversionResultLocal(8-27)
packages/infra/lib/api-stack/fhir-converter-connector.ts (1)
packages/infra/lib/shared/sqs.ts (1)
provideAccessToQueue(219-233)
packages/api/src/command/medical/document/document-query.ts (1)
packages/api/src/domain/medical/conversion-progress.ts (1)
calculateConversionProgress(10-26)
🪛 Biome (1.9.4)
packages/infra/lib/api-stack.ts
[error] 578-579: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (69)
packages/infra/lib/shared/sqs.ts (1)
37-37: Good update to type definitionMaking
lambdaLayersoptional in theQueuePropstype correctly reflects how it's used in the code - the implementation already handles undefined values with the nullish coalescing operator on line 98:props.lambdaLayers ?? [].packages/api/src/routes/internal/medical/docs.ts (2)
147-149: Properly implemented optional count parameterThe implementation correctly extracts and parses the count parameter from the query string, gracefully handling the case when it's not provided.
165-165: Correctly passed count parameter to functionThe parsed count parameter is properly passed to the
calculateDocumentConversionStatusfunction, supporting the batching functionality from the PR objectives.packages/core/src/command/conversion-result/conversion-result-factory.ts (1)
1-15: Well-implemented factory pattern with environment-specific handlersThe factory function provides a clean way to create the appropriate conversion result handler based on the environment. The implementation correctly:
- Uses Config.isDev() to determine the environment
- Creates ConversionResultLocal for development environments
- Creates ConversionResultCloud for production/other environments
- Properly handles environment variables with getEnvVarOrFail
This design facilitates the queuing mechanism referenced in the PR objectives by abstracting away the details of how conversion results are handled in different environments.
packages/lambdas/src/conversion-result-notifier.ts (3)
16-32: Well-structured Lambda handler with proper error trackingThe handler is implemented correctly and follows best practices:
- Wraps with Sentry for error tracking
- Logs the number of incoming records
- Early returns for empty events
- Properly maps and processes the records
- Uses the sendConversionResults function to handle the grouping logic
This implementation aligns with the PR objective of grouping messages based on patient ID, source, and status.
34-41: Good schema definition with ZodThe schema properly defines the expected structure of conversion results with appropriate field types and optionality. This ensures type safety and validation.
43-49: Robust parsing function with error handlingThe parseBody function properly handles:
- Missing body validation
- Type checking
- JSON parsing
- Schema validation
This ensures that only valid data is processed by the Lambda.
packages/lambdas/src/shared/oss-api.ts (1)
1-27: LGTM: Clean removal of the notifyApi functionalityThe code has been cleanly refactored to remove the API notification functionality, which aligns with the PR objective of implementing a queuing mechanism for conversion results. This change is part of moving away from direct API calls for notifications to a more scalable approach using SQS for grouping messages.
packages/core/src/command/conversion-result/types.ts (3)
1-9: Well-structured type definition for ConversionResultThe
ConversionResulttype is well-defined with clear property names and types. It supports the PR objective of grouping messages by patient ID, source (HIE), and status.
11-13: Good extension with optional count propertyAdding the optional count property to extend the base
ConversionResulttype is a good approach, allowing for flexibility in tracking the number of conversions while maintaining backward compatibility.
15-17: Clean interface definition for ConversionResultHandlerThe interface appropriately defines the contract for handling conversion results with a clear
notifyApimethod signature. This provides a consistent way to handle notifications across different implementations (local vs. cloud).packages/core/src/command/conversion-result/conversion-result-cloud.ts (1)
7-12: Well-implemented cloud handler classThe
ConversionResultCloudclass correctly implements theConversionResultHandlerinterface with a clean constructor that initializes an SQS client with the provided region and queue URL.packages/api/src/external/hie/__tests__/tally-doc-query-progress.test.ts (8)
9-46: Well-structured test setup with reusable test functionThe test helper function
testItis well-designed to reduce duplication across test cases and clearly validate the expected outcomes. Good use of default parameters for common test scenarios.
47-62: LGTM: Good test case for partial progressThis test case correctly verifies that the processing status is maintained when adding a success count that's still below the total requirement.
64-79: LGTM: Good test case for exact completionThis test case correctly verifies that the status changes to completed when the success count exactly matches the total.
81-96: LGTM: Good test case for handling existing errorsThis test case correctly verifies that processing status is maintained when there are existing errors, even when adding more successes.
98-113: LGTM: Good test case for completion with errorsThis test case correctly verifies that status changes to completed when the sum of successes and errors equals the total.
115-130: LGTM: Good test case for multiple successesThis test case correctly verifies handling of multiple successes in a single input.
132-147: LGTM: Good test case for mixed successes and errorsThis test case correctly verifies handling of both successes and errors in a single input, leading to completion.
149-159: LGTM: Good edge case testThis test case correctly verifies the handling of undefined existing progress, ensuring default values are applied appropriately.
packages/api/src/command/medical/document/document-query.ts (4)
178-179: Good addition of optional parameters.The addition of optional
countandlogparameters to theUpdateResulttype enhances the flexibility of the function. This allows for more detailed tracking and logging.
185-186: Good default initialization for log parameter.Using the
outfunction as a default provides consistent logging and helps with debugging.
196-198: Useful logging enhancement.Adding this log statement before calculating conversion progress helps with debugging and provides visibility into the state before modifications.
202-202: Properly passing the count parameter.The code now correctly passes the
countparameter tocalculateConversionProgress, which aligns with the function's updated signature as shown in the context snippets.packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (4)
25-25: Address the TODO comment in a future PR.The TODO suggests a better approach for grouping - by patientId only with source/status in an array. This would indeed be a more flexible design.
Consider creating a ticket to address this TODO in a future PR as it would require more extensive changes.
28-40: Good implementation of request grouping.The code effectively groups requests by patient, source, and status, then creates a single request with a count. This aligns well with the PR objective of grouping messages.
42-61: Well-handled error management.The code properly handles errors without disrupting the processing of other requests, which is important for batch operations. The error context logging is also thorough.
35-35: Good error check for programming errors.Checking for the first request and throwing a typed error helps catch programming errors early.
packages/lambdas/src/sqs-to-converter.ts (6)
2-2: Good use of factory pattern for handler creation.Using the
buildConversionResultHandlerfactory function helps with dependency injection and makes the code more testable.
56-56: Proper initialization of the conversion result handler.Creating the handler once at the module level is efficient and follows the singleton pattern appropriately.
146-146: Consistent use of the new handler for failed conversion notification.The code properly replaces the direct API call with the new handler, maintaining consistency.
177-177: Consistent use of the new handler for empty document notification.Similar to the previous comment, this change maintains consistency with the new approach.
324-324: Consistent use of the new handler for error notification.The code properly replaces the direct API call in the error handling section.
469-475: Well-structured successful conversion notification.The code properly replaces the direct API call for successful conversion, maintaining a consistent structure with the rest of the changes.
packages/core/src/command/conversion-result/__tests__/send-multiple-conversion-results.test.ts (1)
1-187: Well-structured and comprehensive test suite for the new conversion result grouping feature.The test suite thoroughly validates the functionality of the
sendConversionResultsfunction, with excellent coverage of various scenarios:
- Single result handling
- Grouping by patient/source/status
- Multiple patient handling
- Error handling and graceful recovery
- Empty input handling
- Different combinations of sources and statuses
This aligns perfectly with the PR objectives to group messages based on patient ID, source (HIE), and status.
packages/api/src/external/hie/tally-doc-query-progress.ts (6)
12-12: Good addition of structured logging.The import of
outfrom the logging utility enables contextual logging that will make debugging easier.
14-14: Appropriate type export for reusability.Exporting the
DynamicProgresstype is a good refactoring choice, improving modularity by allowing this type to be reused across the codebase.
22-22: Flexible logging configuration with proper typing.The addition of the optional
logparameter with proper typing (typeof console.log) allows for custom logging to be injected, enhancing flexibility and testability.
38-38: Clear default logging with context.Using the
oututility to create a contextual logger with patient ID and cxId provides valuable context for troubleshooting, while the default parameter ensures the function remains flexible.
47-48: Enhanced debugging with pre-update status logging.Adding logging of the document query progress state before updates makes debugging easier and provides better visibility into the processing flow.
110-110: Fixed edge case handling for empty external data.As noted in a previous comment, this line fixes the issue where total count wasn't being set for empty external data, ensuring consistent behavior regardless of data state.
packages/api/src/domain/medical/conversion-progress.ts (5)
10-18: Enhanced function signature with count parameter for batch processing.Changing the function declaration style and adding the optional
countparameter improves the flexibility of the conversion progress calculation, supporting the PR's objective to group messages by specified criteria.
23-23: Proper parameter passing to support batch counts.Passing the count parameter to
tallyDocQueryProgressensures consistent counting throughout the conversion pipeline, enabling accurate progress tracking for grouped messages.
28-41: Well-documented function with clear parameter descriptions.The addition of JSDoc comments clearly explains the purpose, parameters, and return value of the function, which greatly improves code maintainability and developer experience.
42-43: Type-safe implementation of count parameter.The function properly defines the parameter types and implements the count parameter with default value handling, ensuring backward compatibility while extending functionality.
46-46: Batch-aware calculation for successful and failed counts.The implementation correctly applies the count parameter when tallying both successful and failed conversions, ensuring accurate progress tracking for batched operations.
Also applies to: 49-49
packages/infra/lib/api-stack.ts (3)
328-333: Lambda architecture refactored for improved queuing infrastructure.The code now uses the lambda directly from the converter connector object rather than creating it separately, streamlining the architecture and reducing the potential for configuration mismatches.
573-574: Environment configuration for conversion result processing.Adding the API URL to both the converter lambda and the new notifier lambda ensures they can communicate with the API service, supporting the queuing mechanism described in the PR objectives.
582-582: Simplified bucket permissions for converter lambda.Granting read-only access to the medical documents bucket for the FHIR converter lambda follows the principle of least privilege, as this lambda only needs to read documents for conversion.
packages/api/src/command/medical/document/document-conversion-status.ts (5)
26-35: Add optionalcountparameter to enhance flexibility.Adding the
count?: numberproperty and passingcountParaminto the function is a good enhancement, as it allows more precise control over how many documents succeed or fail in each update. This ensures more flexible progress tracking.
50-50: Constructing patient object inline is clean.Replacing the need to fetch/create patient objects externally with an inline structure here keeps the function more self-contained.
53-53: Conditional increments for success or error.Using
(convertResult === "success" ? { successful: count } : { errors: count })is straightforward, ensuring the correct tally is updated. This aligns well with the newcountparameter.
125-125: Consistent usage ofcountparameter.Passing
counttoupdateConversionProgresspreserves consistency across all conversion status updates and ensures progress calculations remain accurate.
136-136: Deferred consolidation works as intended.Invoking
recreateConsolidatedasynchronously for patients without a recognized source is logical. No issues found here.packages/infra/lib/lambdas-nested-stack.ts (4)
8-22: New imports for Lambda/SQS integration.Importing
SqsEventSource,Queue, andcreateQueueis correct and aligns with the addition of the new SQS-based notifier function. No issues here.
52-52: IntroducingconversionResultNotifierLambda.Declaring a dedicated property clarifies the stack's responsibilities and ensures easy references to the new Lambda function for other infrastructure components.
127-134: Setting up conversion result notifier connector.Creating the result notifier via
this.setupConversionResultNotifierand saving both the queue and Lambda reference is a solid approach that keeps the stack organized and decoupled.
135-145: SQS queue integration for FHIR converter.Passing
conversionResultNotifierQueuetofhirConverterConnector.createensures all conversion results funnel into the new notifier for processing. This is a clean extension to the existing architecture.packages/api/src/command/medical/document/__tests__/document-query_calculateAndUpdateDocQuery.test.ts (4)
76-83: NewcalculateConversionProgresstest suite withcountparameter.Renaming the test suite and introducing the
countparameter helps validate more nuanced scenarios, ensuring the code handles multiple documents correctly.
101-112: Extended assertions for dynamic success/error increments.Including
countin the calculation and verifying it in the tests (lines 109, 112) ensures the logic corresponds exactly to the parameter's value for both successes and failures.
122-139: Comprehensive success cases with variouscountvalues.These new test cases confirm that the status transitions from
"processing"to"completed"correctly when multiple successes accumulate to match or exceedtotal. This boosts confidence in the updated logic.
161-165: Enhanced failure scenarios with multi-doc increments.Testing partial and full failure increments (using
count) ensures robust coverage of negative outcomes and consistent progress status updates.packages/infra/lib/api-stack/fhir-converter-connector.ts (7)
10-10: New import for environment configuration.The addition of the
EnvConfigimport is necessary for handling the environment configuration that will be passed to thecreatefunction. This aligns with the goal of passing configuration cleanly through the system.
22-22: Good enhancement to the FHIRConverterConnector type.Adding the
lambda: Lambdaproperty to theFHIRConverterConnectortype is a good change as it explicitly tracks the associated Lambda function, providing more comprehensive type safety and improving integration with other components.
65-134: Well-structured encapsulation of connector creation.The new
createfunction provides excellent encapsulation of the FHIR Converter connector creation process. The comprehensive JSDoc comments clearly explain the purpose and parameters of the function. This change improves modularity and maintainability by bringing together all the necessary components for creating a complete connector.
136-146: Improved type safety with Omit return type.The updated return type
Omit<FHIRConverterConnector, "lambda">correctly reflects that this function only creates the queue, DLQ, and bucket, but not the Lambda function. This enhances type safety and makes the API more explicit.
202-202: New parameter for API notification.The addition of the
apiNotifierQueueparameter to thecreateLambdafunction aligns with the PR's objective to implement a queuing mechanism after the FHIR converter, enabling the grouping of messages based on specified criteria.Also applies to: 216-217
245-245: Enhanced environment variable naming.Changing from
API_SERVICE_DNS_ADDRESStoCONVERSION_RESULT_QUEUE_URLprovides a more descriptive environment variable name that better represents its purpose - to identify the queue where conversion results should be sent.
266-266: Proper queue access control.Adding permission for the Lambda function to send messages to the API notifier queue follows the principle of least privilege, granting only the necessary access required for operation. This is good security practice.
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/utils/src/fhir-converter/s3-to-lambda.ts (3)
7-7: Cross-package import may be brittleUsing a relative path that crosses package boundaries (
../../../api/src/external/fhir-converter/converter) could lead to issues if file locations change. Consider using proper package imports instead.-import { convertCDAToFHIR } from "../../../api/src/external/fhir-converter/converter"; +import { convertCDAToFHIR } from "@metriport/api/external/fhir-converter/converter";
29-47: Replace console.log with proper logging utilityThe script uses direct
console.logstatements instead of a structured logging utility, which would be more consistent with the rest of the codebase (such as theout()utility used in converter.ts).+import { out } from "@metriport/core/util"; async function main() { + const { log, error } = out("s3-to-lambda"); await sleep(50); // Give some time to avoid mixing logs w/ Node's const startedAt = Date.now(); - console.log(`############## Started at ${new Date(startedAt).toISOString()} ##############`); + log(`############## Started at ${new Date(startedAt).toISOString()} ##############`); - console.log(`Converting ${fileNames.length} files...`); + log(`Converting ${fileNames.length} files...`); // ... rest of function - console.log(`>>>>>>> Done after ${elapsedTimeAsStr(startedAt)}`); + log(`>>>>>>> Done after ${elapsedTimeAsStr(startedAt)}`); }
25-27: Consider using a command-line argument parser for better flexibilityThe current script has hardcoded values that would need to be manually edited for each use. Using a command-line argument parser would make the script more reusable and flexible.
+import { Command } from 'commander'; +const program = new Command(); +program + .option('-c, --cx-id <id>', 'Customer ID') + .option('-p, --patient-id <id>', 'Patient ID') + .option('-f, --files <files>', 'Comma-separated list of file names to process') + .parse(process.argv); + +const options = program.opts(); -const cxId = ""; -const patientId = ""; -const fileNames: string[] = []; +const cxId = options.cxId || process.env.CX_ID || ""; +const patientId = options.patientId || process.env.PATIENT_ID || ""; +const fileNames: string[] = options.files ? options.files.split(',') : []; const bucket = Config.getMedicalDocumentsBucketName();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
packages/api/src/external/fhir-converter/converter.ts(1 hunks)packages/utils/src/fhir-converter/s3-to-lambda.ts(1 hunks)
🧰 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/api/src/external/fhir-converter/converter.tspackages/utils/src/fhir-converter/s3-to-lambda.ts
🧬 Code Definitions (1)
packages/utils/src/fhir-converter/s3-to-lambda.ts (1)
packages/api/src/external/fhir-converter/converter.ts (1)
convertCDAToFHIR(38-86)
🔇 Additional comments (1)
packages/api/src/external/fhir-converter/converter.ts (1)
40-40: Parameter signature simplified by removing the optional content propertyThe
documentparameter has been simplified by removing the optionalcontentproperty of typeContentMimeType. This change aligns with the implementation since the function only uses thedocument.idproperty and doesn't referencedocument.contentanywhere in its body.This modification supports the PR objective of implementing a queuing mechanism after FHIR conversion, as it simplifies the interface between components.
packages/core/src/command/conversion-result/conversion-result-cloud.ts
Outdated
Show resolved
Hide resolved
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts
Outdated
Show resolved
Hide resolved
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (1)
68-70: 🛠️ Refactor suggestionCreate helper methods for key encoding/decoding.
As noted in a previous review, helper methods for creating and retrieving keys would improve maintainability. The current implementation doesn't handle edge cases like having the separator character in any of the values.
function encodeResultKey(result: ConversionResult): string { - return `${result.patientId}${keySeparator}${result.source}${keySeparator}${result.status}`; + // Ensure values exist and handle potential separator characters in the values + const patientId = result.patientId || ""; + const source = result.source || ""; + const status = result.status || ""; + return [patientId, source, status].join(keySeparator); }
🧹 Nitpick comments (5)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (3)
8-8: Create a descriptive constant name.The constant
keySeparatoris used for encoding and decoding keys, but its name doesn't fully convey its purpose. Consider renaming to something more descriptive that indicates its role in grouping conversion results.-const keySeparator = "|"; +const RESULT_GROUP_KEY_SEPARATOR = "|";
36-37: Simplify redundant null check.The check for
!firstRequestseems unnecessary since the loop wouldn't execute if there were no grouped requests. ThegroupedRequests[0]access is already safe within this context.- const firstRequest = groupedRequests[0]; - if (!firstRequest) throw new MetriportError(`Programming error: missing first request`); + // This array will always have at least one element due to how groupBy works + const firstRequest = groupedRequests[0];
60-60: Avoid multi-line logs per coding guidelines.The log statement combines multiple pieces of information into a single line, which violates the guideline to avoid multi-line logs. Consider restructuring to use multiple log calls or a more structured approach.
- log(`${msg}, error: ${errorToString(error)}, extra: ${JSON.stringify(extra)}`); + log(`${msg}, error: ${errorToString(error)}`); + log(`Context: ${extra.context}, patientId: ${extra.patientId}, status: ${extra.status}`);packages/core/src/command/conversion-result/conversion-result-cloud.ts (2)
11-14: Add logging to notifyApi method.Unlike the
ConversionResultLocalclass which includes logging, this implementation doesn't log the notification process. Consider adding logging using theout().logapproach consistent with the rest of the codebase.async notifyApi(params: ConversionResult): Promise<void> { + const { cxId, patientId, jobId } = params; + const { log } = out(`notifyApi.cloud - cx ${cxId} patient ${patientId} job ${jobId}`); + log(`Sending message to SQS queue: ${this.conversionResultQueueUrl}`); const payload = JSON.stringify(params); await this.sqsClient.sendMessageToQueue(this.conversionResultQueueUrl, payload); }
11-14: Implement network retries for SQS messages.The
ConversionResultLocalclass usesexecuteWithNetworkRetriesfor API calls, but this implementation doesn't retry SQS messages. Consider adding retry logic for consistency and reliability.async notifyApi(params: ConversionResult): Promise<void> { const payload = JSON.stringify(params); - await this.sqsClient.sendMessageToQueue(this.conversionResultQueueUrl, payload); + await executeWithNetworkRetries( + () => this.sqsClient.sendMessageToQueue(this.conversionResultQueueUrl, payload), + { + retryOnTimeout: true, + maxAttempts: MAX_API_NOTIFICATION_ATTEMPTS, + } + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
packages/core/src/command/conversion-result/conversion-result-cloud.ts(1 hunks)packages/core/src/command/conversion-result/send-multiple-conversion-results.ts(1 hunks)
🧰 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/command/conversion-result/conversion-result-cloud.tspackages/core/src/command/conversion-result/send-multiple-conversion-results.ts
🧬 Code Definitions (2)
packages/core/src/command/conversion-result/conversion-result-cloud.ts (1)
packages/core/src/command/conversion-result/types.ts (2)
ConversionResultHandler(15-17)ConversionResult(1-9)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (2)
packages/core/src/command/conversion-result/types.ts (2)
ConversionResult(1-9)ConversionResultWithCount(11-13)packages/core/src/command/conversion-result/conversion-result-local.ts (1)
ConversionResultLocal(8-27)
🔇 Additional comments (1)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (1)
27-27: Consider implementing the suggested improvement.The TODO comment suggests a better approach to group results by patientId only. This might be worth discussing as a follow-up improvement since it aligns with the PR objectives of grouping messages by patient ID, source, and status.
Would this proposed change reduce the complexity or improve performance of the current implementation? What are the specific server-side changes that would be needed?
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (3)
27-28: Consider tracking TODO as a separate issue.The TODO comment indicates a potential future improvement. Consider creating a ticket to track this enhancement and reference it in the comment to ensure it's not forgotten.
- // TODO group this by patientId only, with the count move to an array of source/status + // TODO: group this by patientId only, with the count move to an array of source/status - see ticket #XXXX
36-42: Consider using a more robust approach for handling the first request.While the current implementation works correctly, consider a more descriptive error message or using the first request more explicitly for better code clarity.
const firstRequest = groupedRequests[0]; - if (!firstRequest) throw new MetriportError(`Programming error: missing first request`); + if (!firstRequest) { + throw new MetriportError(`Programming error: missing first request for patient`, undefined, { + extra: { patientId, source, status }, + }); + }
44-62: Avoid multi-line logs per coding guidelines.According to the coding guidelines, we should avoid multi-line logs. Consider restructuring the error logging to comply with this guideline.
try { await conversionResultHandler.notifyApi(request, log); resultsWithCount.push(request); } catch (error) { // Can't bubble up because we might have multiple patients, which mean multiple // requests to the API, and we don't know which one was processed or failed. // Bubbling up would send the message to DLQ and we can't reprocess it b/c of // the above. const msg = `Error processing conversion result notification`; const extra = { context: "conversion-result-notifier", patientId, status, requests: groupedRequests, error, }; - log(`${msg}, error: ${errorToString(error)}, extra: ${JSON.stringify(extra)}`); + log(`${msg}, error: ${errorToString(error)}`); capture.error(msg, { extra }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts(1 hunks)
🧰 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/command/conversion-result/send-multiple-conversion-results.ts
🧬 Code Definitions (1)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (2)
packages/core/src/command/conversion-result/types.ts (2)
ConversionResult(1-9)ConversionResultWithCount(11-13)packages/core/src/command/conversion-result/conversion-result-local.ts (1)
ConversionResultLocal(8-27)
🔇 Additional comments (4)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (4)
1-9: LGTM! Appropriate imports and initialization.The imports look correct for the functionality being implemented, and the
keySeparatorconstant is well-defined for use in the encoding/decoding functions.
10-23: Function is well-documented with appropriate signature.Good job on the clear documentation explaining the purpose of the function and its parameters.
68-70: LGTM! Effective key encoding implementation.The function creates a composite key by concatenating patient ID, source, and status with a separator, which is simple and effective.
72-84: LGTM! Robust key decoding with proper error handling.Good job implementing the error handling for invalid key formats as suggested in the previous review.
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref. metriport/metriport-internal#1040 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
da1fb31 to
91d5d44
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (1)
27-27: Clarify or remove the TODO comment.The code groups by
patientId | source | status, but there's a TODO to group by patientId only, which may cause confusion. Consider updating the comment or removing it if the current design is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
packages/api/src/external/carequality/document/process-outbound-document-retrieval-resps.ts(1 hunks)packages/core/src/command/conversion-result/send-multiple-conversion-results.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/external/carequality/document/process-outbound-document-retrieval-resps.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/command/conversion-result/send-multiple-conversion-results.ts
🧬 Code Definitions (1)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (2)
packages/core/src/command/conversion-result/types.ts (2)
ConversionResult(1-9)ConversionResultWithCount(11-13)packages/core/src/command/conversion-result/conversion-result-local.ts (1)
ConversionResultLocal(8-27)
🔇 Additional comments (2)
packages/core/src/command/conversion-result/send-multiple-conversion-results.ts (2)
68-84: Handle unexpected pipe characters in keys.If
patientId,source, orstatuscan contain the pipe (|) character, it will break the encoding/decoding logic. Consider either validating these fields and throwing an error early or using a safer delimiter or encoding approach.Do you want a helper to safely URL-encode these fields before grouping?
1-85: Overall implementation looks solid.The file effectively groups results, logs them, and notifies the API with a batched request. Async error handling is handled gracefully without throwing for each batch. No major performance or maintainability issues noticed. Great job!
Ticket: https://github.com/metriport/metriport-internal/issues/1040
Dependencies
none
Description
Add queue after FHIR converter - context.
Group messages by:
We can later batch only by patient ID, but that would require more changes on the server (riskier) that I didn't want to tackle now.
Testing
Release Plan
Summary by CodeRabbit
New Features
countto the/conversion-statusendpoint.Refactor