ENG-268 Add seed/copy consolidate script #3859
Conversation
WalkthroughThe shared package's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SeedScript
participant AWS_S3
User->>SeedScript: Run seed-patient-consolidated.ts
SeedScript->>User: Display warning and confirmation prompt
User->>SeedScript: Confirm operation
SeedScript->>AWS_S3: List files with source prefix (medical and conversion buckets)
loop For each file (parallel up to 30)
SeedScript->>AWS_S3: Download file (if JSON) or copy file (if not JSON)
alt JSON file
SeedScript->>SeedScript: Replace source patient ID with destination patient ID
SeedScript->>AWS_S3: Upload modified JSON to destination path
else Non-JSON file
SeedScript->>AWS_S3: Direct S3 copy to destination path
end
end
SeedScript->>User: Log progress and completion
sequenceDiagram
participant User
participant CheckScript
participant AWS_S3
User->>CheckScript: Run check-patient-consolidated.ts with cxId, patientId, optional file path
alt Local file path provided
CheckScript->>CheckScript: Read bundle JSON from local file
else No local file path
CheckScript->>AWS_S3: Download bundle JSON from S3 bucket
end
CheckScript->>CheckScript: Parse JSON as FHIR Bundle
CheckScript->>CheckScript: Run checkBundle validation
CheckScript->>User: Log validation results and elapsed time
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 warn config production Use ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/utils/src/consolidated/seed-patient-consolidated.ts (4)
13-13: Remove or explain commented importThe commented import suggests functionality that might be relevant but is not being used.
Either remove the commented import or add a comment explaining why it's kept but commented out:
-// import { isMimeTypeXML } from "@metriport/core/util/mime"; +// XML processing temporarily disabled - will be implemented in future version
54-54: Remove unused commented codeThis line appears to be leftover development code that's not used.
- // const destinationPath = createFolderName(sourceCxId, sourcePatientId);
76-76: Remove or explain commented functionalityThere's commented code that suggests XML files might need special handling.
- // if (isMimeTypeXML(fileType)) return; + // Skip XML files processing for now - will be implemented in ENG-XXX
110-110: Consider making parallel execution count configurableThe hard-coded value of 30 parallel executions might not be optimal for all environments or workloads.
- { numberOfParallelExecutions: 30 } + { numberOfParallelExecutions: Number(process.env.PARALLEL_EXECUTIONS) || 30 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/shared/package.json(2 hunks)packages/utils/src/consolidated/seed-patient-consolidated.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/utils/src/consolidated/seed-patient-consolidated.ts
⏰ 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: Analyze (javascript)
🔇 Additional comments (3)
packages/shared/package.json (2)
27-27: Well-structured addition to exports patternThe addition of the medical subpath export pattern follows the established convention in the file and will allow consumers to directly import from the medical directory.
60-62: Type declarations properly addedThe TypeScript type declarations for the medical subpath are correctly configured, maintaining consistency with other subpaths in the package.
packages/utils/src/consolidated/seed-patient-consolidated.ts (1)
57-58: 🛠️ Refactor suggestionUse out().log consistently instead of console.log
According to the coding guidelines, direct use of console.log should be avoided in favor of out().log.
- console.log(`>>> Processing bucket ${bucketName}...`); + log(`>>> Processing bucket ${bucketName}...`); - console.log(`... found ${sourceFiles.length} files to copy`); + log(`... found ${sourceFiles.length} files to copy`); - console.log(`... copied ${sourceFileKey} to ${destinationFileKey}`); + log(`... copied ${sourceFileKey} to ${destinationFileKey}`);Also applies to: 60-60, 108-108
⛔ Skipped due to learnings
Learnt from: leite08 PR: metriport/metriport#3489 File: packages/api/src/routes/ehr/elation/appointment-webhook.ts:32-36 Timestamp: 2025-03-21T00:21:26.928Z Learning: Use `out().log` instead of `console.log` for logging in packages other than utils, infra and shared.Learnt from: thomasyopes PR: metriport/metriport#3466 File: packages/api/src/routes/ehr/shared.ts:122-147 Timestamp: 2025-03-17T17:01:17.227Z Learning: Avoid using console.log and console.error in packages other than utils, infra and shared, and try to use out().log instead according to Metriport's coding guidelines.
d5f2f4b to
5384386
Compare
| // STOP! Never point these to production buckets! | ||
| const medicalDocsBucketName = getEnvVarOrFail("MEDICAL_DOCUMENTS_BUCKET_NAME"); | ||
| const conversionsBucketName = getEnvVarOrFail("CONVERSION_RESULT_BUCKET_NAME"); | ||
|
|
There was a problem hiding this comment.
maybe add some validation here in case something gets pointed to prod, we catch it?
if (medicalDocsBucketName.toLowerCase().includes("prod") || conversionsBucketName.toLowerCase().includes("prod")) throw some exception
There was a problem hiding this comment.
Approving, but address @husainsyed's comments first. I like the one about prod warning
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
5384386 to
453b030
Compare
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/utils/src/consolidated/seed-patient-consolidated.ts (3)
25-28: 🛠️ Refactor suggestionMake IDs configurable through command-line arguments
Empty string variables for source and destination IDs require manual file editing before use, which reduces usability and makes the script harder to maintain.
Consider using command-line arguments or environment variables instead:
-const sourceCxId = ""; -const sourcePatientId = ""; -const destinationCxId = ""; -const destinationPatientId = ""; +const sourceCxId = process.env.SOURCE_CX_ID || process.argv[2]; +const sourcePatientId = process.env.SOURCE_PATIENT_ID || process.argv[3]; +const destinationCxId = process.env.DESTINATION_CX_ID || process.argv[4]; +const destinationPatientId = process.env.DESTINATION_PATIENT_ID || process.argv[5]; + +if (!sourceCxId || !sourcePatientId || !destinationCxId || !destinationPatientId) { + console.error("Missing required parameters. Usage:"); + console.error("ts-node src/consolidated/seed-patient-consolidated.ts <sourceCxId> <sourcePatientId> <destinationCxId> <destinationPatientId>"); + process.exit(1); +}
129-134: 🛠️ Refactor suggestionEnhance confirmation mechanism for better safety
The current confirmation is passive - it just waits for a fixed time period without requiring explicit user confirmation. This could lead to unintended operations if the script is started accidentally.
Consider implementing an active confirmation mechanism:
async function displayWarningAndConfirmation(log: typeof console.log) { const msg = `You are about to copy the consolidated data of patient ${sourcePatientId} to ${destinationPatientId}.`; log(msg); - log("Cancel this now if you're not sure."); - await sleep(confirmationTime.asMilliseconds()); + log("Type 'CONFIRM' to proceed or press Ctrl+C to cancel..."); + + return new Promise<void>((resolve) => { + const stdin = process.stdin; + stdin.setRawMode(true); + stdin.resume(); + stdin.setEncoding("utf8"); + + let input = ""; + const string) => { + // ctrl-c + if (data === "\u0003") { + log("Operation cancelled"); + process.exit(0); + } + + // Backspace + if (data === "\u007f" && input.length > 0) { + input = input.substring(0, input.length - 1); + process.stdout.write("\b \b"); + return; + } + + if (data === "\r" || data === "\n") { + process.stdout.write("\n"); + if (input === "CONFIRM") { + stdin.removeListener("data", onData); + stdin.setRawMode(false); + stdin.pause(); + resolve(); + } else { + log("Invalid confirmation. Operation cancelled."); + process.exit(1); + } + return; + } + + input += data; + process.stdout.write(data); + }; + + stdin.on("data", onData); + }); }
59-60: 🛠️ Refactor suggestionAdd a validation check for identical source and destination
The script should prevent accidentally copying a patient's data onto itself, which could potentially cause data corruption or unexpected behavior.
const sourceFilenamePrefix = createFilePath(sourceCxId, sourcePatientId, ""); const destinationFilenamePrefix = createFilePath(destinationCxId, destinationPatientId, ""); + + // Prevent copying to the same patient + if (sourceCxId === destinationCxId && sourcePatientId === destinationPatientId) { + log("ERROR: Source and destination patient IDs are identical. Operation aborted."); + process.exit(1); + }
🧹 Nitpick comments (2)
packages/utils/src/consolidated/seed-patient-consolidated.ts (2)
63-68: Use consistent logging approachThe script mixes direct
console.logcalls with the structured logger created viaout(""). Using a consistent logging approach improves maintainability and readability.async function copyData(bucketName: string) { - console.log(`>>> Processing bucket ${bucketName}...`); + log(`>>> Processing bucket ${bucketName}...`); const sourceFiles = await s3Utils.listObjects(bucketName, sourceFilenamePrefix); if (sourceFiles.length < 1) return; - console.log(`... found ${sourceFiles.length} files to copy`); + log(`... found ${sourceFiles.length} files to copy`); // Later in the function - console.log(`... copied ${sourceFileKey} to ${destinationFileKey}`); + log(`... copied ${sourceFileKey} to ${destinationFileKey}`);Also applies to: 115-115
117-117: Consider adjusting the parallelism level based on expected workloadSetting
numberOfParallelExecutionsto 30 could potentially cause AWS API rate limiting issues if there are many files to process.- { numberOfParallelExecutions: 30 } + { numberOfParallelExecutions: 10 }Consider a more conservative value initially, or make this configurable through an environment variable:
+const parallelExecutions = parseInt(process.env.PARALLEL_EXECUTIONS || "10", 10); // Then use it later - { numberOfParallelExecutions: 30 } + { numberOfParallelExecutions: parallelExecutions }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
packages/utils/src/consolidated/seed-patient-consolidated.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/utils/src/consolidated/seed-patient-consolidated.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
packages/utils/src/consolidated/seed-patient-consolidated.ts (1)
Learnt from: leite08
PR: metriport/metriport#3412
File: packages/utils/src/patient-import/send-webhook.ts:41-55
Timestamp: 2025-04-24T02:17:02.661Z
Learning: Files under `packages/utils` don't need robust error handling as they are utility scripts meant for local/developer environments, not production code.
🧬 Code Graph Analysis (1)
packages/utils/src/consolidated/seed-patient-consolidated.ts (3)
packages/core/src/external/aws/s3.ts (1)
S3Utils(114-509)packages/shared/src/index.ts (1)
sleep(13-13)packages/core/src/util/concurrency.ts (1)
executeAsynchronously(83-130)
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/utils/src/consolidated/check-patient-consolidated.ts (2)
38-40: Remove or document the purpose of the sleep call.The
sleep(100)call at the beginning of the main function seems arbitrary and its purpose isn't clear. It adds a delay without any explanation.Either remove this sleep call if it's unnecessary or add a comment explaining its purpose:
async function main() { - await sleep(100); + // If sleep is necessary, add a comment explaining why initRunsFolder();
17-27: Add more detailed usage instructions in the documentation.The documentation explains the purpose of the script but doesn't provide details on how to configure it before running.
Enhance the documentation with more detailed usage instructions:
/** * This script checks a patient's consolidated data using the checkBundle() function * from packages/core. * * Either set the bundleFilePath or the cxId and patientId. * + * Configuration: + * - bundleFilePath: Path to a local JSON file containing the bundle + * - cxId: Customer ID when downloading from S3 + * - patientId: Patient ID when downloading from S3 + * + * Environment variables required: + * - MEDICAL_DOCUMENTS_BUCKET_NAME: S3 bucket name for medical documents + * - AWS_REGION: AWS region for S3 operations * * Execute this with: * $ ts-node src/consolidated/check-patient-consolidated.ts + * + * Or with arguments: + * $ ts-node src/consolidated/check-patient-consolidated.ts [bundleFilePath] [cxId] [patientId] */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/utils/src/consolidated/check-patient-consolidated.ts(1 hunks)packages/utils/src/consolidated/seed-patient-consolidated.ts(1 hunks)packages/utils/src/fhir/fhir-converter/convert-and-generate-mr.ts(1 hunks)packages/utils/src/fhir/fhir-converter/convert.ts(1 hunks)packages/utils/src/fhir/fhir-converter/count-bundle-refs.ts(1 hunks)packages/utils/src/fhir/fhir-converter/count-resources.ts(1 hunks)packages/utils/src/fhir/fhir-converter/integration-test.ts(1 hunks)packages/utils/src/fhir/fhir-converter/s3-to-lambda.ts(1 hunks)packages/utils/src/fhir/fhir-converter/shared.ts(1 hunks)packages/utils/src/fhir/fhir-converter/validate-fhir-bundles.ts(1 hunks)packages/utils/src/fhir/fhir-converter/validator/validate-converted-xmls.ts(1 hunks)packages/utils/src/fhir/fhir-deduplication/report/get-files.ts(1 hunks)packages/utils/src/fhir/fhir-deduplication/report/report-dedup.ts(1 hunks)packages/utils/src/fhir/fhir-hydration/hydrate-files-local.ts(1 hunks)packages/utils/src/fhir/fhir-normalization/normalize-files-local.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/utils/src/fhir/fhir-converter/s3-to-lambda.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/utils/src/fhir/fhir-converter/integration-test.ts
- packages/utils/src/fhir/fhir-converter/shared.ts
- packages/utils/src/fhir/fhir-converter/validator/validate-converted-xmls.ts
- packages/utils/src/fhir/fhir-converter/convert.ts
- packages/utils/src/fhir/fhir-deduplication/report/get-files.ts
- packages/utils/src/fhir/fhir-normalization/normalize-files-local.ts
- packages/utils/src/fhir/fhir-converter/count-resources.ts
- packages/utils/src/fhir/fhir-deduplication/report/report-dedup.ts
- packages/utils/src/fhir/fhir-converter/count-bundle-refs.ts
- packages/utils/src/fhir/fhir-converter/convert-and-generate-mr.ts
- packages/utils/src/fhir/fhir-converter/validate-fhir-bundles.ts
- packages/utils/src/fhir/fhir-hydration/hydrate-files-local.ts
- packages/utils/src/consolidated/seed-patient-consolidated.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/utils/src/consolidated/check-patient-consolidated.ts
⏰ 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: Analyze (javascript)
🔇 Additional comments (3)
packages/utils/src/consolidated/check-patient-consolidated.ts (3)
1-16: LGTM! Import and configuration setup looks good.The imports are well-organized with dotenv config correctly placed at the top. The dayjs plugin extension is properly set up.
33-37: LGTM! AWS and S3 setup is correct.The S3 setup appropriately uses environment variables to get the bucket name and region.
62-65: LGTM! Elapsed time measurement and clean exit.The elapsed time calculation and logging is a good practice for script performance tracking.
| try { | ||
| log(`>>> Checking bundle...`); | ||
| checkBundle(bundle, cxId, patientId); | ||
| } catch (error) { | ||
| log(`>>> Error: ${error}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and exit with appropriate code.
The script catches errors during bundle checking but continues execution and exits with code 0 (success) regardless of whether errors occurred. This could be misleading for automated processes that rely on exit codes.
Enhance the error handling to provide more context and exit with a non-zero code on failure:
try {
log(`>>> Checking bundle...`);
checkBundle(bundle, cxId, patientId);
+ log(`>>> Bundle check completed successfully`);
} catch (error) {
- log(`>>> Error: ${error}`);
+ log(`>>> Error during bundle validation: ${error}`);
+ // Exit with error code
+ process.exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| log(`>>> Checking bundle...`); | |
| checkBundle(bundle, cxId, patientId); | |
| } catch (error) { | |
| log(`>>> Error: ${error}`); | |
| } | |
| try { | |
| log(`>>> Checking bundle...`); | |
| checkBundle(bundle, cxId, patientId); | |
| log(`>>> Bundle check completed successfully`); | |
| } catch (error) { | |
| log(`>>> Error during bundle validation: ${error}`); | |
| // Exit with error code | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In packages/utils/src/consolidated/check-patient-consolidated.ts around lines 55
to 60, the current error handling logs the error but does not stop execution or
exit with a failure code. Modify the catch block to log the error with more
context and then exit the process with a non-zero exit code to indicate failure,
ensuring automated systems can detect the error properly.
There was a problem hiding this comment.
No need for error handling on util scripts
There was a problem hiding this comment.
@leite08 Understood! I see from our previous discussions that utility scripts in the packages/utils directory have different requirements compared to production code and don't need comprehensive error handling. I'll keep this in mind for future reviews of utility scripts.
🐰
( ^.^ )
>
🧠 Learnings used
Learnt from: leite08
PR: metriport/metriport#3412
File: packages/utils/src/patient-import/send-webhook.ts:41-55
Timestamp: 2025-04-24T02:17:02.661Z
Learning: Files under `packages/utils` don't need robust error handling as they are utility scripts meant for local/developer environments, not production code.
Learnt from: leite08
PR: metriport/metriport#3611
File: packages/utils/src/carequality/download-directory.ts:68-82
Timestamp: 2025-04-05T16:02:22.473Z
Learning: Utility scripts in the packages/utils directory don't require comprehensive error handling like production code.
Learnt from: leite08
PR: metriport/metriport#3595
File: packages/utils/src/carequality/download-directory.ts:36-66
Timestamp: 2025-04-03T02:23:58.332Z
Learning: In the metriport codebase, utility scripts may not require the same level of error handling as production code, especially for simple or one-off tasks like in the CQ directory download script.
Learnt from: leite08
PR: metriport/metriport#3611
File: packages/utils/src/carequality/download-directory.ts:33-66
Timestamp: 2025-04-05T16:02:31.219Z
Learning: In the Metriport codebase, utility scripts in the packages/utils directory have different error handling requirements compared to production code, and do not necessarily need the same level of error handling.
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
d4faadb to
733bbee
Compare
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/utils/src/s3/calculate-consolidated-size-in-s3.ts (2)
41-41: Consider using more descriptive logging.Following the coding guidelines for meaningful names and to align with the logging pattern used in the updated
file-history.tsscript.- console.log(`${Object.keys(cxIds).length} cxs, ${cxAndPatient.length} patients`); + console.log(`${Object.keys(cxIds).length} customers, ${cxAndPatient.length} patients`);
27-27: Consider providing example data for the cxAndPatient array.The array is currently empty and needs to be populated by users. Consider adding example entries as comments to help users understand the expected format.
-const cxAndPatient: string[][] = []; +const cxAndPatient: string[][] = [ + // ["customer-id-1", "patient-id-1"], + // ["customer-id-2", "patient-id-2"], +];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/utils/src/s3/calculate-consolidated-size-in-s3.ts(1 hunks)packages/utils/src/s3/file-history.ts(6 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/utils/src/s3/calculate-consolidated-size-in-s3.tspackages/utils/src/s3/file-history.ts
⏰ 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: Analyze (javascript)
🔇 Additional comments (1)
packages/utils/src/s3/file-history.ts (1)
32-32: LGTM! Good improvements to concurrency and logging.The changes improve the script's performance and user experience:
- Increased parallel calls from 10 to 50 for better performance
- Added timing information with consistent formatting
- Added progress logging per customer
- Used proper constants for configuration
These changes align well with the approach taken in the new
calculate-consolidated-size-in-s3.tsscript.Also applies to: 52-52, 69-69, 82-82
| function inGigabyteStr(bytes: number): string { | ||
| return (bytes / 1_048_576).toFixed(2) + " GB"; | ||
| } |
There was a problem hiding this comment.
Fix incorrect byte to gigabyte conversion.
The function divides by 1_048_576 (1 MB) but labels the result as GB. The correct conversion should divide by 1_073_741_824 (1 GB).
Apply this fix:
function inGigabyteStr(bytes: number): string {
- return (bytes / 1_048_576).toFixed(2) + " GB";
+ return (bytes / 1_073_741_824).toFixed(2) + " GB";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function inGigabyteStr(bytes: number): string { | |
| return (bytes / 1_048_576).toFixed(2) + " GB"; | |
| } | |
| function inGigabyteStr(bytes: number): string { | |
| return (bytes / 1_073_741_824).toFixed(2) + " GB"; | |
| } |
🤖 Prompt for AI Agents
In packages/utils/src/s3/calculate-consolidated-size-in-s3.ts at lines 92 to 94,
the function inGigabyteStr incorrectly converts bytes to gigabytes by dividing
by 1,048,576 (1 MB) instead of 1,073,741,824 (1 GB). Fix this by changing the
divisor to 1,073,741,824 to correctly convert bytes to gigabytes while keeping
the rest of the function unchanged.
| for (const [cxId, entries] of Object.entries(cxIds)) { | ||
| let cxSize = 0; | ||
| const patientIds = entries.map(v => v[1]); | ||
| totalPatients = +patientIds.length; |
There was a problem hiding this comment.
Fix incorrect totalPatients calculation.
The current implementation reassigns totalPatients in each loop iteration instead of accumulating. This will result in totalPatients only reflecting the last customer's patient count.
Apply this fix:
- totalPatients = +patientIds.length;
+ totalPatients += patientIds.length;Note: The unary + operator is also unnecessary since patientIds.length is already a number.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| totalPatients = +patientIds.length; | |
| - totalPatients = +patientIds.length; | |
| + totalPatients += patientIds.length; |
🤖 Prompt for AI Agents
In packages/utils/src/s3/calculate-consolidated-size-in-s3.ts at line 50, the
code incorrectly assigns totalPatients to patientIds.length in each iteration,
overwriting the previous value. To fix this, change the assignment to accumulate
the count by adding patientIds.length to totalPatients instead of reassigning
it. Also, remove the unnecessary unary plus operator since patientIds.length is
already a number.
| await sleep(50); // Give some time to avoid mixing logs w/ Node's | ||
| console.log(`############ Starting... ############`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Follow coding guidelines for logging.
The coding guidelines specify to avoid using console.log in packages other than utils, infra and shared, and try to use out().log instead.
Consider importing and using the proper logging utility:
+import { out } from "@metriport/core/util/out";
async function main() {
await sleep(50); // Give some time to avoid mixing logs w/ Node's
- console.log(`############ Starting... ############`);
+ out().log(`############ Starting... ############`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await sleep(50); // Give some time to avoid mixing logs w/ Node's | |
| console.log(`############ Starting... ############`); | |
| import { out } from "@metriport/core/util/out"; | |
| async function main() { | |
| await sleep(50); // Give some time to avoid mixing logs w/ Node's | |
| out().log(`############ Starting... ############`); | |
| // …rest of main() | |
| } |
🤖 Prompt for AI Agents
In packages/utils/src/s3/calculate-consolidated-size-in-s3.ts at lines 36 to 37,
replace the console.log statement with the appropriate logging utility as per
coding guidelines. Import the logging function (e.g., out().log) from the
designated logging module and use it to output the "Starting..." message instead
of console.log to maintain consistency and adhere to the project's logging
standards.
| @@ -48,9 +49,10 @@ type Result = { | |||
| async function main() { | |||
| await sleep(50); // Give some time to avoid mixing logs w/ Node's | |||
| console.log(`############ Starting... ############`); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Follow coding guidelines for logging.
Similar to the other script, this file uses console.log which violates the coding guidelines that specify to avoid using console.log in packages other than utils, infra and shared.
Consider updating all console.log calls to use the proper logging utility:
+import { out } from "@metriport/core/util/out";
// Replace all console.log calls with out().log
- console.log(`############ Starting... ############`);
+ out().log(`############ Starting... ############`);Also applies to: 55-55, 71-71, 82-82
🤖 Prompt for AI Agents
In packages/utils/src/s3/file-history.ts at lines 51, 55, 71, and 82, replace
all console.log calls with the appropriate logging utility as per the coding
guidelines. Identify the logging utility used in this project (e.g., a logger
instance or a logging module) and update each console.log statement to use that
instead, ensuring consistent and guideline-compliant logging throughout the
file.
Dependencies
none
Description
Testing
Release Plan
Summary by CodeRabbit