ENG 484 - Medication dispense deduplication, Surescripts bug fixes #4161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#2995 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
|
Warning Rate limit exceeded@keshavsaharia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis set of changes introduces new features, refactors FHIR resource handling, and adds CLI utilities. Key updates include new API modules for recreating consolidated bundles, deduplication logic for MedicationDispense resources, removal of context/deduplication utilities from FHIR conversion, enhanced CLI tools for batch and facility requests, and CSV/XML parsing utilities. Several function signatures are updated to favor explicit parameter passing over context objects. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant DataMapper
participant API
participant Axios
participant SFTPClient
CLI->>DataMapper: recreateConsolidatedBundle(cxId, patientId)
DataMapper->>API: recreateConsolidatedBundle({cxId, patientId}, axiosInstance)
API->>Axios: POST /recreate-consolidated?cxId&patientId
Axios-->>API: { requestId }
API-->>DataMapper: { requestId }
DataMapper-->>CLI: { requestId }
sequenceDiagram
participant CLI
participant SFTPClient
CLI->>SFTPClient: sendBatchPatientRequest(requests[])
SFTPClient->>SFTPClient: Connect
loop for each patient
SFTPClient->>SFTPClient: Generate request, send via SFTP
end
SFTPClient->>SFTPClient: Disconnect
SFTPClient-->>CLI: [fileIdentifiers]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
keshavsaharia
commented
Jul 8, 2025
| @@ -62,7 +63,6 @@ export function dangerouslyDeduplicateFhir( | |||
| "medicationAdministrations", | |||
| "medicationStatements", | |||
| "medicationRequests", | |||
| "medicationDispenses", | |||
There was a problem hiding this comment.
Not sure why this is in the array twice, so I removed one.
keshavsaharia
commented
Jul 8, 2025
| @@ -645,6 +650,16 @@ function replaceResourceReference<T extends Resource>( | |||
| } | |||
| } | |||
|
|
|||
| if ("authorizingPrescription" in entry && Array.isArray(entry.authorizingPrescription)) { | |||
There was a problem hiding this comment.
Maintains the reference to a MedicationRequest resource.
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#2995 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#2995 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/utils/src/surescripts/shared.ts (1)
41-49: Security concern: Avoid shell command injection vulnerability.The
execSynccommand with string interpolation is vulnerable to command injection if the file path contains special characters.Apply this diff to fix the security issue:
-import { execSync } from "child_process"; +import { execSync } from "child_process"; +import { shell } from "execa"; export function openSurescriptsRunsFile(filePath: string): void { const dir = getSurescriptsDirOrFail(); const fullFilePath = path.join(dir, filePath); if (fs.existsSync(fullFilePath)) { - execSync(`open ${fullFilePath}`); + execSync("open", [fullFilePath]); } else { console.error(`File not found: ${fullFilePath}`); } }Alternatively, use the
openpackage for cross-platform compatibility:import open from "open"; export async function openSurescriptsRunsFile(filePath: string): Promise<void> { const dir = getSurescriptsDirOrFail(); const fullFilePath = path.join(dir, filePath); if (fs.existsSync(fullFilePath)) { await open(fullFilePath); } else { console.error(`File not found: ${fullFilePath}`); } }
🧹 Nitpick comments (10)
packages/core/src/external/surescripts/file/xml-parser.ts (1)
3-7: Consider using async/await for consistency.Per the coding guidelines, prefer
async/awaitover returning promises directly.-export function parseXmlResponse(responseFileContent: Buffer): Promise<unknown> { - const xml = responseFileContent.toString(); - const result = parseStringPromise(xml); - return result; -} +export async function parseXmlResponse(responseFileContent: Buffer): Promise<unknown> { + const xml = responseFileContent.toString(); + return await parseStringPromise(xml); +}packages/core/src/external/surescripts/fhir-converter.ts (1)
12-15: Use proper logging instead of console.log.The XML detection logic is sound and appropriately prevents processing XML files in this converter. However, the logging should follow coding guidelines.
Replace
console.logwith proper logging:- console.log("Response file is XML! Skipping conversion."); + // Note: XML files are handled by separate XML parserOr if logging is needed, use the appropriate logging utility for core packages.
packages/utils/src/surescripts/convert-response-to-csv.ts (2)
8-14: Remove unused interface properties.The
ConvertResponseOptionsinterface defines several optional properties (cxId,facilityId,populationId) that are never used in the action function. This creates unnecessary complexity and potential confusion.Apply this diff to clean up the interface:
-interface ConvertResponseOptions { - cxId?: string; - facilityId?: string; - transmissionId?: string; - populationId?: string; - patientId?: string; -} +interface ConvertResponseOptions { + transmissionId?: string; + patientId?: string; +}
23-35: Improve error message specificity.The action function logic is sound, but the error message on line 29 could be more descriptive to help with debugging.
Apply this diff to improve the error message:
- if (!file) throw new Error("File not found"); + if (!file) throw new Error(`Response file not found for transmission ID: ${transmissionId}, patient ID: ${patientId}`);packages/core/src/external/surescripts/api/recreate-consolidated.ts (1)
21-26: Fix misleading JSDoc comment.The JSDoc comment incorrectly states "retrieve the patient" when the function actually recreates/refreshes a consolidated bundle.
Apply this diff to correct the documentation:
/** - * Sends an API request to retrieve the patient with the given ID. + * Sends an API request to recreate the consolidated bundle for the given patient. * * @param params - The customer ID and patient ID - * @returns patient demographic data + * @returns the request ID for the consolidation operation */packages/core/src/external/surescripts/client.ts (1)
305-324: Address logging inconsistency and clarify file selection logic.The changes improve handling of multiple response files, but there are a few concerns:
- Logging inconsistency: Uses
console.loginstead ofthis.log(line 314-321), which is inconsistent with the rest of the class.- Unclear latest file logic: The assumption that the last file in the array is the "latest" may not be correct unless the files are sorted by timestamp.
Apply this diff to address the logging inconsistency:
- console.log( + this.log( "Multiple responses for " + transmissionId + " and " + populationId + ": " + replicatedResponses.join(", ") - ); + );Consider whether the file selection logic should sort by timestamp or use a more explicit method to determine the "latest" file.
packages/utils/src/surescripts/convert-customer-response.ts (1)
73-89: Consider adding concurrency control for API calls.The recreation logic is well-implemented with proper error handling and progress tracking. However, consider adding a concurrency limit to
executeAsynchronouslyto prevent potential API rate limiting issues when processing many patients.- await executeAsynchronously(transmissions, async ({ patientId }) => { + await executeAsynchronously(transmissions, async ({ patientId }) => { try { const result = await dataMapper.recreateConsolidatedBundle(cxId, patientId); console.log( `Recreated consolidated bundle for ${patientId} with request ID ${result.requestId}` ); recreatedCount++; } catch (error) { console.error(`Error recreating consolidated bundle for ${patientId}: ${error}`); } - }); + }, { + numberOfParallelExecutions: 10, + });packages/core/src/external/surescripts/file/file-parser.ts (1)
29-53: Consider improving CSV string escaping and handling edge cases.The CSV parsing function is well-implemented with good type handling. However, consider these improvements:
- String escaping: String values should escape internal quotes to prevent CSV parsing issues.
- Object handling: JSON.stringify for objects could create very large CSV cells.
- if (typeof value === "string") return `"${value}"`; + if (typeof value === "string") return `"${value.replace(/"/g, '""')}"`; if (typeof value === "number") return value.toString(); if (typeof value === "boolean") return value ? "true" : "false"; - if (typeof value === "object") return JSON.stringify(value); + if (typeof value === "object") return `"${JSON.stringify(value).replace(/"/g, '""')}"`;packages/core/src/fhir-deduplication/resources/medication-dispense.ts (1)
75-117: Consider refactoring repetitive deduplication logic.The grouping logic contains repetitive patterns that could be extracted into a helper function to improve maintainability.
+function createKeyAndDeduplicate( + keyObj: object, + medDispense: MedicationDispense, + medDispensesMap: Map<string, MedicationDispense>, + refReplacementMap: Map<string, string> +): void { + const key = JSON.stringify(keyObj); + deduplicateWithinMap({ + dedupedResourcesMap: medDispensesMap, + dedupKey: key, + candidateResource: medDispense, + refReplacementMap, + onPremerge: preprocessStatus, + }); +} for (const medDispense of medDispenses) { const medRef = medDispense.medicationReference?.reference; const date = medDispense.whenHandedOver; const daysSupply = medDispense.daysSupply; const quantity = medDispense.quantity; if (medRef && date && daysSupply) { const datetime = getDateFromString(date, "datetime"); - const key = JSON.stringify({ medRef, datetime, daysSupply }); - deduplicateWithinMap({ - dedupedResourcesMap: medDispensesMap, - dedupKey: key, - candidateResource: medDispense, - refReplacementMap, - onPremerge: preprocessStatus, - }); + createKeyAndDeduplicate({ medRef, datetime, daysSupply }, medDispense, medDispensesMap, refReplacementMap); } else if (medRef && date && quantity) { const datetime = getDateFromString(date, "datetime"); - const key = JSON.stringify({ medRef, datetime, quantity }); - deduplicateWithinMap({ - dedupedResourcesMap: medDispensesMap, - dedupKey: key, - candidateResource: medDispense, - refReplacementMap, - onPremerge: preprocessStatus, - }); + createKeyAndDeduplicate({ medRef, datetime, quantity }, medDispense, medDispensesMap, refReplacementMap); } else if (medRef && date) { const datetime = getDateFromString(date, "datetime"); - const key = JSON.stringify({ medRef, datetime }); - deduplicateWithinMap({ - dedupedResourcesMap: medDispensesMap, - dedupKey: key, - candidateResource: medDispense, - refReplacementMap, - onPremerge: preprocessStatus, - }); + createKeyAndDeduplicate({ medRef, datetime }, medDispense, medDispensesMap, refReplacementMap); } else if (medRef) { - const key = JSON.stringify({ medRef }); - deduplicateWithinMap({ - dedupedResourcesMap: medDispensesMap, - dedupKey: key, - candidateResource: medDispense, - refReplacementMap, - onPremerge: preprocessStatus, - }); + createKeyAndDeduplicate({ medRef }, medDispense, medDispensesMap, refReplacementMap); } else { danglingReferences.add(createRef(medDispense)); } }packages/utils/src/surescripts/shared.ts (1)
129-129: Remove debug console.log statement.The console.log statement appears to be debug code that should be removed.
- console.log(file.Key);
Comment on lines
+104
to
+105
| system: PLAN_NETWORK_PCN_SYSTEM, | ||
| type: { id: "BIN" }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that both PCN and BIN identifiers should use the same system.
The change makes both planNetworkPCN and planNetworkBIN identifiers use PLAN_NETWORK_PCN_SYSTEM. Please verify this is intentionally correct, as PCN (Processor Control Number) and BIN (Bank Identification Number) are typically different types of network identifiers that might require different system identifiers.
🏁 Script executed:
#!/bin/bash
# Search for PLAN_NETWORK_BIN_SYSTEM usage to see if it was intentionally replaced
rg "PLAN_NETWORK_BIN_SYSTEM" --type tsLength of output: 134
I’d like to confirm the actual values of both constants to ensure they’re not inadvertently identical. Let’s pull their definitions:
🏁 Script executed:
#!/bin/bash
# Show definitions and context for both systems
rg -n -C2 "PLAN_NETWORK_BIN_SYSTEM" packages/core/src/external/surescripts/fhir/constants.ts
rg -n -C2 "PLAN_NETWORK_PCN_SYSTEM" packages/core/src/external/surescripts/fhir/constants.tsLength of output: 746
Here’s the incorrect use of the PCN system for a BIN identifier. The BIN should use its own system constant.
🏁 Script executed:
#!/bin/bash
# Show the planNetworkBIN identifier in coverage.ts
rg -n -C3 "planNetworkBIN" packages/core/src/external/surescripts/fhir/coverage.tsLength of output: 383
Use the correct system constant for the BIN identifier
The BIN identifier should reference PLAN_NETWORK_BIN_SYSTEM instead of PLAN_NETWORK_PCN_SYSTEM. Please update in packages/core/src/external/surescripts/fhir/coverage.ts at the BIN block:
• File: packages/core/src/external/surescripts/fhir/coverage.ts
Lines: around 104–105
Proposed change:
if (detail.planNetworkBIN) {
identifiers.push({
- system: PLAN_NETWORK_PCN_SYSTEM,
+ system: PLAN_NETWORK_BIN_SYSTEM,
type: { id: "BIN" },
value: detail.planNetworkBIN?.toString() ?? "",
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| system: PLAN_NETWORK_PCN_SYSTEM, | |
| type: { id: "BIN" }, | |
| if (detail.planNetworkBIN) { | |
| identifiers.push({ | |
| system: PLAN_NETWORK_BIN_SYSTEM, | |
| type: { id: "BIN" }, | |
| value: detail.planNetworkBIN?.toString() ?? "", | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/external/surescripts/fhir/coverage.ts around lines 104 to
105, the system constant for the BIN identifier is incorrectly set to
PLAN_NETWORK_PCN_SYSTEM. Update the system value to use PLAN_NETWORK_BIN_SYSTEM
instead to correctly reference the BIN identifier.
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
packages/utils/src/surescripts/compare/dr-first.ts (1)
315-336: Optimize RxNorm crosswalk with concurrent execution.The function processes NDC codes sequentially, which could be slow for large datasets.
async function getDrFirstRxNorms( drFirstEvents: HistoryData["events"], metriportRxNorms: Set<string> ): Promise<{ drFirstRxNorms: Set<string>; sharedRxNorms: Set<string> }> { const drFirstRxNorms = new Set<string>(); const sharedRxNorms = new Set<string>(); - for (const event of drFirstEvents) { - if (event.medicationNdc) { - const ndc = event.medicationNdc; - const rxNorm = await crosswalkNdcToRxNorm(ndc); - if (rxNorm && rxNorm.code) { - drFirstRxNorms.add(rxNorm.code); - if (metriportRxNorms.has(rxNorm.code)) { - sharedRxNorms.add(rxNorm.code); - } - } - } - } + const ndcCodes = drFirstEvents + .filter(event => event.medicationNdc) + .map(event => event.medicationNdc); + + const rxNormPromises = ndcCodes.map(ndc => crosswalkNdcToRxNorm(ndc)); + const rxNormResults = await Promise.all(rxNormPromises); + + rxNormResults.forEach((rxNorm, index) => { + if (rxNorm && rxNorm.code) { + drFirstRxNorms.add(rxNorm.code); + if (metriportRxNorms.has(rxNorm.code)) { + sharedRxNorms.add(rxNorm.code); + } + } + }); return { drFirstRxNorms, sharedRxNorms }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/utils/src/surescripts/compare/convert-dr-first.ts(1 hunks)packages/utils/src/surescripts/compare/dr-first.ts(1 hunks)packages/utils/src/surescripts/compare/history-table.ts(1 hunks)packages/utils/src/surescripts/convert-response-to-csv.ts(1 hunks)packages/utils/src/surescripts/index.ts(1 hunks)packages/utils/src/surescripts/shared.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/utils/src/surescripts/convert-response-to-csv.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/utils/src/surescripts/index.ts
- packages/utils/src/surescripts/shared.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Don’t use null inside the app, only on code interacting with external interfaces/services, like DB and HTTP; convert to undefined before sending inwards into the code
Useconstwhenever possible
Useasync/awaitinstead of.then()
Naming: classes, enums: PascalCase
Naming: constants, variables, functions: camelCase
Naming: file names: kebab-case
Naming: Don’t use negative names, likenotEnabled, preferisDisabled
If possible, use decomposing objects for function parameters
Prefer Nullish Coalesce (??) than the OR operator (||) when you want to provide a default value
Avoid creating arrow functions
Use truthy syntax instead ofin- i.e.,if (data.link)notif ('link' in data)
While handling errors, keep the stack trace around: if you create a new Error (e.g., MetriportError), make sure to pass the original error as the new one’s cause so the stack trace is available upstream.
max column length is 100 chars
multi-line comments use/** */
top-level comments go after the import (save pre-import to basic file header, like license)
move literals to constants declared after imports when possible
Files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/history-table.tspackages/utils/src/surescripts/compare/dr-first.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursorrules)
Use types whenever possible
Files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/history-table.tspackages/utils/src/surescripts/compare/dr-first.ts
**/*.ts
⚙️ CodeRabbit Configuration File
**/*.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
- U...
Files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/history-table.tspackages/utils/src/surescripts/compare/dr-first.ts
🧠 Learnings (22)
📓 Common learnings
Learnt from: keshavsaharia
PR: metriport/metriport#4075
File: packages/core/src/external/surescripts/fhir/coverage.ts:18-38
Timestamp: 2025-06-25T18:45:28.839Z
Learning: In the Surescripts FHIR bundle generation, deduplication of insurance organizations is handled at the bundle-entry level in the getCoverageResources function using the deduplicateByKey function, rather than in individual resource creation functions like getInsuranceOrganization. The coverageOrganization property in SurescriptsContext is used to track deduplicated organizations.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.
Learnt from: keshavsaharia
PR: metriport/metriport#4045
File: packages/core/src/external/surescripts/fhir/coverage.ts:0-0
Timestamp: 2025-06-18T18:34:10.489Z
Learning: Coverage resources in Surescripts FHIR conversion are currently excluded from bundles to prevent skewing data lift metrics. The team plans to examine available insurance data thoroughly before including properly structured Coverage resources with mandatory FHIR R4 elements like beneficiary and payor references.
Learnt from: lucasdellabella
PR: metriport/metriport#3781
File: packages/core/src/fhir-deduplication/shared.ts:58-72
Timestamp: 2025-05-06T01:54:38.058Z
Learning: In the FHIR deduplication process, when merging resources in `mergeIntoTargetResource`, only the extensions from the target resource plus a reference extension to the source are intentionally preserved, discarding any extensions unique to the source resource. This maintains parity with the previous implementation's behavior.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: thomasyopes
PR: metriport/metriport#3936
File: packages/core/src/external/ehr/athenahealth/index.ts:472-482
Timestamp: 2025-06-06T15:37:44.571Z
Learning: In AthenaHealth medication creation, even if medication statements have the same start/stop dates, they should not be deduplicated because there may be relevant differences beyond dates (dosage, administration routes, prescriber info, etc.) that make each statement unique and worth creating separately.
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Learnt from: keshavsaharia
PR: metriport/metriport#4075
File: packages/core/src/external/surescripts/fhir/medication-request.ts:76-83
Timestamp: 2025-06-25T18:42:07.231Z
Learning: In packages/core/src/external/surescripts/fhir/medication-request.ts, the getDispenseNote and getDosageInstruction functions are intentionally identical because the dashboard requires both the note and dosageInstruction fields of MedicationRequest to be populated with detail.directions for proper note display functionality.
Learnt from: thomasyopes
PR: metriport/metriport#3981
File: packages/core/src/external/ehr/canvas/index.ts:650-657
Timestamp: 2025-06-11T16:56:22.035Z
Learning: In Canvas write-back (`createMedicationStatements`), the intended business logic is to submit only the most recent medication statement; older statements should be ignored.
Learnt from: leite08
PR: metriport/metriport#4043
File: packages/core/src/command/consolidated/consolidated-create.ts:0-0
Timestamp: 2025-06-18T00:18:59.144Z
Learning: In the consolidated bundle creation process (packages/core/src/command/consolidated/consolidated-create.ts), the team prefers strict failure behavior where if any component (including pharmacy resources) fails to be retrieved, the entire consolidation should fail rather than gracefully degrading. This ensures data consistency and prevents incomplete bundles.
Learnt from: keshavsaharia
PR: metriport/metriport#4058
File: packages/core/src/external/quest/fhir/bundle.ts:68-76
Timestamp: 2025-07-31T17:11:00.860Z
Learning: When reviewing code that involves transformations to FHIR resources, always search the codebase for existing FHIR utility functions (such as UUID validation, URL wrapping, reference generation, etc.) to avoid code duplication and ensure consistency with established patterns.
Learnt from: lucasdellabella
PR: metriport/metriport#4181
File: packages/api/src/command/medical/patient/batch-utils.ts:63-78
Timestamp: 2025-07-17T00:19:09.217Z
Learning: In batch processing utilities like packages/api/src/command/medical/patient/batch-utils.ts, user lucasdellabella prefers to keep the utility focused on its core responsibility (batching) and delegate retry logic to the calling code, rather than implementing retry mechanisms within the utility itself.
Learnt from: thomasyopes
PR: metriport/metriport#4039
File: packages/core/src/external/surescripts/fhir/medication.ts:0-0
Timestamp: 2025-06-19T21:51:26.432Z
Learning: In Surescripts FHIR conversion code, numeric validation for fields like detail.quantityDispensed is not considered necessary - the team prefers not to add defensive validation for NaN values in these conversions.
📚 Learning: in packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts, the conversionfhirdirect cla...
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts:18-25
Timestamp: 2025-06-25T01:56:08.732Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts, the ConversionFhirDirect class is designed for local testing purposes and does not require comprehensive error handling for HTTP requests, as confirmed by the user thomasyopes.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the fhir converter lambda end...
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:30-30
Timestamp: 2025-06-25T01:55:42.627Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the FHIR converter Lambda endpoint is controlled by the team and guaranteed to return valid JSON in the expected Bundle<Resource> format, so JSON.parse() error handling and type validation are not necessary for this specific endpoint.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: the functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is ...
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.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/history-table.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: the writemedicationtochart function in packages/api/src/external/ehr/athenahealth/command/write-back...
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/history-table.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: for utility scripts in the packages/utils directory, developers are expected to update the main cons...
Learnt from: leite08
PR: metriport/metriport#3859
File: packages/utils/src/consolidated/check-patient-consolidated.ts:28-32
Timestamp: 2025-05-26T16:24:10.245Z
Learning: For utility scripts in the packages/utils directory, developers are expected to update the main constants at the top of the file (like bundleFilePath, cxId, patientId) rather than implementing command-line argument parsing or environment variable reading. This is the preferred pattern for configuring utility scripts in this codebase.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in packages/core/src/external/sftp/surescripts/client.ts, the standalone getpatientloadfilename func...
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/sftp/surescripts/client.ts:1-134
Timestamp: 2025-05-28T19:23:20.179Z
Learning: In packages/core/src/external/sftp/surescripts/client.ts, the standalone getPatientLoadFileName function intentionally omits the transmission ID from the filename, which differs from the class method getPatientLoadFileName. This difference in filename generation is expected behavior.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: user leite08 acknowledged the duplicate inputbundle spread issue in packages/core/src/command/analyt...
Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.ts
📚 Learning: in packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveadtconversion...
Learnt from: lucasdellabella
PR: metriport/metriport#3907
File: packages/core/src/external/fhir/adt-encounters.ts:118-143
Timestamp: 2025-05-28T02:32:27.527Z
Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.ts
📚 Learning: in packages/api/src/routes/internal/analytics-platform/index.ts, the patientid parameter handling is...
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.581Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.ts
📚 Learning: in packages/core/src/external/surescripts/client.ts, the surescripts date formatting should use loca...
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/surescripts/client.ts:0-0
Timestamp: 2025-05-30T01:09:55.013Z
Learning: In packages/core/src/external/surescripts/client.ts, the Surescripts date formatting should use local time according to their documentation ("message-date-must-be-local"), which means buildDayjs() cannot be used since it defaults to UTC. The specific use case for transmission date formatting requires dayjs to be used directly to preserve local time behavior.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts, the team...
Learnt from: thomasyopes
PR: metriport/metriport#4061
File: packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts:34-47
Timestamp: 2025-06-19T22:44:49.393Z
Learning: In packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts, the team prefers to keep refreshEhrBundles operations grouped using fire-and-forget pattern rather than awaiting all operations with Promise.allSettled(). This allows individual refresh operations to run independently without blocking the job runner.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in the consolidated bundle creation process (packages/core/src/command/consolidated/consolidated-cre...
Learnt from: leite08
PR: metriport/metriport#4043
File: packages/core/src/command/consolidated/consolidated-create.ts:0-0
Timestamp: 2025-06-18T00:18:59.144Z
Learning: In the consolidated bundle creation process (packages/core/src/command/consolidated/consolidated-create.ts), the team prefers strict failure behavior where if any component (including pharmacy resources) fails to be retrieved, the entire consolidation should fail rather than gracefully degrading. This ensures data consistency and prevents incomplete bundles.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-typ...
Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:27-49
Timestamp: 2025-05-28T19:22:09.281Z
Learning: In packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in batch processing utilities like packages/api/src/command/medical/patient/batch-utils.ts, user luc...
Learnt from: lucasdellabella
PR: metriport/metriport#4181
File: packages/api/src/command/medical/patient/batch-utils.ts:63-78
Timestamp: 2025-07-17T00:19:09.217Z
Learning: In batch processing utilities like packages/api/src/command/medical/patient/batch-utils.ts, user lucasdellabella prefers to keep the utility focused on its core responsibility (batching) and delegate retry logic to the calling code, rather than implementing retry mechanisms within the utility itself.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.ts
📚 Learning: in the metriport codebase, the team prefers to let errors bubble up naturally in some cases rather t...
Learnt from: thomasyopes
PR: metriport/metriport#3788
File: packages/api/src/external/ehr/shared/utils/bundle.ts:83-93
Timestamp: 2025-05-08T19:41:36.533Z
Learning: In the Metriport codebase, the team prefers to let errors bubble up naturally in some cases rather than adding explicit error handling at every layer, as demonstrated in the refreshEhrBundle function in the bundle.ts file.
Applied to files:
packages/utils/src/surescripts/compare/convert-dr-first.ts
📚 Learning: in packages/core/src/external/surescripts/fhir/medication-request.ts, the getdispensenote and getdos...
Learnt from: keshavsaharia
PR: metriport/metriport#4075
File: packages/core/src/external/surescripts/fhir/medication-request.ts:76-83
Timestamp: 2025-06-25T18:42:07.231Z
Learning: In packages/core/src/external/surescripts/fhir/medication-request.ts, the getDispenseNote and getDosageInstruction functions are intentionally identical because the dashboard requires both the note and dosageInstruction fields of MedicationRequest to be populated with detail.directions for proper note display functionality.
Applied to files:
packages/utils/src/surescripts/compare/history-table.tspackages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientdata.firstna...
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts:10-22
Timestamp: 2025-06-27T01:50:14.227Z
Learning: In packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientData.firstName, patientData.lastName, and patientData.dob are guaranteed to be non-nullable values, so defensive null/undefined checks are not needed when accessing these fields.
Applied to files:
packages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: in v0 of the surescripts implementation (packages/utils/src/surescripts/generate-patient-load-file.t...
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/utils/src/surescripts/generate-patient-load-file.ts:92-94
Timestamp: 2025-05-30T01:11:35.300Z
Learning: In V0 of the Surescripts implementation (packages/utils/src/surescripts/generate-patient-load-file.ts), there is always only 1 facility ID, so the current facility_ids parsing logic with substring operations is sufficient and additional validation is not necessary.
Applied to files:
packages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: the circular dependency between shared.ts (importing getpatientidsorfail) and adt/utils.ts (using un...
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Applied to files:
packages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: coverage resources in surescripts fhir conversion are currently excluded from bundles to prevent ske...
Learnt from: keshavsaharia
PR: metriport/metriport#4045
File: packages/core/src/external/surescripts/fhir/coverage.ts:0-0
Timestamp: 2025-06-18T18:34:10.489Z
Learning: Coverage resources in Surescripts FHIR conversion are currently excluded from bundles to prevent skewing data lift metrics. The team plans to examine available insurance data thoroughly before including properly structured Coverage resources with mandatory FHIR R4 elements like beneficiary and payor references.
Applied to files:
packages/utils/src/surescripts/compare/dr-first.ts
📚 Learning: the `getmetriportonlybundlebyresourcetype` method in canvasapi returns `promise`...
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
Applied to files:
packages/utils/src/surescripts/compare/dr-first.ts
🧬 Code Graph Analysis (1)
packages/utils/src/surescripts/compare/history-table.ts (1)
packages/shared/src/common/date.ts (1)
buildDayjs(93-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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/surescripts/compare/convert-dr-first.ts (1)
33-57: Add comprehensive error handling for async operations.The current implementation lacks error handling for critical async operations and throws an error on the first invalid transmission ID, stopping all processing.
Consider wrapping async operations in try-catch blocks and logging errors instead of throwing:
for (const { transmissionId, patientId } of transmissions) { - const transmissionIds = await getAllConversionBundleJobIds(CX_ID, patientId); - if (!transmissionIds.includes(transmissionId)) { - throw new Error(`Invalid transmission ID ${transmissionId}`); - } + try { + const transmissionIds = await getAllConversionBundleJobIds(CX_ID, patientId); + if (!transmissionIds.includes(transmissionId)) { + console.error(`Invalid transmission ID ${transmissionId} for patient ${patientId}`); + continue; + } - const entries: BundleEntry[] = []; - for (const transmissionId of transmissionIds) { - const conversion = await handler.convertPatientResponse({ - cxId: CX_ID, - facilityId: FACILITY_ID, - transmissionId, - populationId: patientId, - }); - if (conversion && conversion.bundle && conversion.bundle.entry) { - entries.push(...conversion.bundle.entry); + const entries: BundleEntry[] = []; + for (const transmissionId of transmissionIds) { + const conversion = await handler.convertPatientResponse({ + cxId: CX_ID, + facilityId: FACILITY_ID, + transmissionId, + populationId: patientId, + }); + if (conversion && conversion.bundle && conversion.bundle.entry) { + entries.push(...conversion.bundle.entry); + } } - } - const bundle = buildBundle({ type: "collection", entries }); - dangerouslyDeduplicateFhir(bundle, CX_ID, patientId); - await writeLatestConversionBundle(CX_ID, patientId, bundle); - const nameId = nameIdMapping[patientId]; - writeLocalBundle(nameId, bundle); - console.log(`Wrote bundle to local directory`); + const bundle = buildBundle({ type: "collection", entries }); + dangerouslyDeduplicateFhir(bundle, CX_ID, patientId); + await writeLatestConversionBundle(CX_ID, patientId, bundle); + const nameId = nameIdMapping[patientId]; + writeLocalBundle(nameId, bundle); + console.log(`Wrote bundle for ${nameId} to local directory`); + } catch (error) { + console.error(`Error processing patient ${patientId}:`, error); + } }⛔ Skipped due to learnings
Learnt from: lucasdellabella PR: metriport/metriport#3866 File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:142-151 Timestamp: 2025-05-27T16:10:48.223Z Learning: In packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts, the user prefers to let axios errors bubble up naturally rather than adding try-catch blocks that re-throw with MetriportError wrappers, especially when the calling code already has retry mechanisms in place via simpleExecuteWithRetries.Learnt from: leite08 PR: metriport/metriport#3814 File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174 Timestamp: 2025-05-20T21:26:26.804Z Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.Learnt from: leite08 PR: metriport/metriport#3944 File: packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts:53-53 Timestamp: 2025-06-01T02:28:19.913Z Learning: In packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts, the processErrors function intentionally throws MetriportError to bubble errors up the call stack rather than handling them locally. This is by design - errors from ingestPatientConsolidated should propagate upward rather than being caught at immediate calling locations.Learnt from: thomasyopes PR: metriport/metriport#4165 File: packages/data-transformation/fhir-to-csv/main.py:0-0 Timestamp: 2025-07-18T09:53:38.906Z Learning: In packages/data-transformation/fhir-to-csv/main.py, S3 upload operations should be allowed to hard fail without try-catch blocks. The team prefers immediate failure over error handling and continuation for S3 upload operations in the FHIR-to-CSV transformation process.Learnt from: thomasyopes PR: metriport/metriport#3788 File: packages/api/src/external/ehr/shared/utils/bundle.ts:83-93 Timestamp: 2025-05-08T19:41:36.533Z Learning: In the Metriport codebase, the team prefers to let errors bubble up naturally in some cases rather than adding explicit error handling at every layer, as demonstrated in the refreshEhrBundle function in the bundle.ts file.Learnt from: thomasyopes PR: metriport/metriport#4090 File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:30-30 Timestamp: 2025-06-25T01:55:42.627Z Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the FHIR converter Lambda endpoint is controlled by the team and guaranteed to return valid JSON in the expected Bundle<Resource> format, so JSON.parse() error handling and type validation are not necessary for this specific endpoint.Learnt from: thomasyopes PR: metriport/metriport#3870 File: packages/core/src/external/ehr/bundle/job/create-resource-diff-bundles/steps/compute/ehr-compute-resource-diff-bundles-local.ts:55-91 Timestamp: 2025-05-19T18:24:41.632Z Learning: In EHR bundles related to resource diff computations, bundle creation operations that are for auditing purposes should not stop the main job flow if they fail. These operations typically use try/catch blocks that log errors without rethrowing them, which is the intended behavior.Learnt from: leite08 PR: metriport/metriport#3940 File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:60-73 Timestamp: 2025-05-31T21:29:39.196Z Learning: In search-consolidated.ts, the user prefers to let errors bubble up from concurrent search operations (Promise.all) rather than catching them explicitly with try-catch blocks. Errors from searchFhirResources and searchDocuments should be allowed to propagate up the call stack.Learnt from: thomasyopes PR: metriport/metriport#4090 File: packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts:18-25 Timestamp: 2025-06-25T01:56:08.732Z Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts, the ConversionFhirDirect class is designed for local testing purposes and does not require comprehensive error handling for HTTP requests, as confirmed by the user thomasyopes.Learnt from: leite08 PR: metriport/metriport#4034 File: packages/core/src/command/patient-import/patient-or-record-failed.ts:19-30 Timestamp: 2025-06-16T17:03:23.069Z Learning: In patient import error handling functions like setPatientOrRecordFailed, the team prefers Promise.all over Promise.allSettled to ensure atomic all-or-nothing behavior - if either the job update or patient record update fails, the entire operation should fail rather than allowing partial success.Learnt from: lucasdellabella PR: metriport/metriport#3907 File: packages/core/src/external/fhir/adt-encounters.ts:118-143 Timestamp: 2025-05-28T02:32:27.527Z Learning: In packages/core/src/external/fhir/adt-encounters.ts, conversion bundles stored by saveAdtConversionBundle do not require version ID assertions or versioning, unlike the sourced encounter data stored by putAdtSourcedEncounter which does require versionId validation.Learnt from: thomasyopes PR: metriport/metriport#4061 File: packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts:34-47 Timestamp: 2025-06-19T22:44:49.393Z Learning: In packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts, the team prefers to keep refreshEhrBundles operations grouped using fire-and-forget pattern rather than awaiting all operations with Promise.allSettled(). This allows individual refresh operations to run independently without blocking the job runner.Learnt from: keshavsaharia PR: metriport/metriport#3885 File: packages/core/src/external/sftp/surescripts/client.ts:1-134 Timestamp: 2025-05-28T19:23:20.179Z Learning: In packages/core/src/external/sftp/surescripts/client.ts, the standalone getPatientLoadFileName function intentionally omits the transmission ID from the filename, which differs from the class method getPatientLoadFileName. This difference in filename generation is expected behavior.Learnt from: leite08 PR: metriport/metriport#4043 File: packages/core/src/command/consolidated/consolidated-create.ts:0-0 Timestamp: 2025-06-18T00:18:59.144Z Learning: In the consolidated bundle creation process (packages/core/src/command/consolidated/consolidated-create.ts), the team prefers strict failure behavior where if any component (including pharmacy resources) fails to be retrieved, the entire consolidation should fail rather than gracefully degrading. This ensures data consistency and prevents incomplete bundles.
keshavsaharia
commented
Aug 2, 2025
| const parsedFileName = parseResponseFileName(fileName); | ||
| return ( | ||
| parsedFileName && | ||
| parsedFileName.transmissionId === transmissionId && | ||
| parsedFileName.populationId === populationId | ||
| ); | ||
| }); | ||
| if (replicatedResponses.length > 2) { |
keshavsaharia
commented
Aug 2, 2025
| let patientIds = await dataMapper.getPatientIdsForFacility({ cxId, facilityId }); | ||
| console.log(`Found ${patientIds.length} patients`); | ||
|
|
||
| if (start) { |
keshavsaharia
commented
Aug 2, 2025
| console.log("Sending " + requests.length + " requests"); | ||
| const identifiers = await client.sendBatchPatientRequest(requests); | ||
| console.log("Done writing facility requests"); | ||
| await client.disconnect(); |
There was a problem hiding this comment.
move to client
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/utils/src/surescripts/send-patient-request.ts (1)
19-36: Consider capturing the return value from sendPatientRequest.The action callback refactoring to use destructured parameters is excellent and more type-safe. However, based on the AI summary, the
sendPatientRequestmethod now returnsPromise<string | undefined>instead ofPromise<void>, but the return value is not being captured or logged.Consider capturing and logging the return value for better visibility:
- await handler.sendPatientRequest({ cxId, facilityId, patientId }); + const result = await handler.sendPatientRequest({ cxId, facilityId, patientId }); + if (result) { + console.log(`Patient request sent successfully: ${result}`); + }packages/utils/src/surescripts/send-batch-request.ts (1)
39-56: Consider consolidating the two different approaches.The code uses two different patterns for similar functionality:
- Lines 40-45: Uses
SurescriptsSendBatchRequestHandlerDirectwith{ cxId, facilityId, patientIds }- Lines 47-53: Uses direct
SurescriptsSftpClientwith{ cxId, facility, patients }This architectural inconsistency may lead to maintenance issues and confusion.
Consider using the handler pattern consistently:
if (patientIds) { const handler = new SurescriptsSendBatchRequestHandlerDirect( new SurescriptsSftpClient({ logLevel: "debug", }) ); await handler.sendBatchRequest({ cxId, facilityId, patientIds: patientIds.split(",") }); } else if (csvData) { - const dataMapper = new SurescriptsDataMapper(); - const facility = await dataMapper.getFacilityData(cxId, facilityId); - const patients = await getPatientsFromCsv(csvData); - const client = new SurescriptsSftpClient({ - logLevel: "debug", - }); - await client.sendBatchRequest({ cxId, facility, patients }); + const patients = await getPatientsFromCsv(csvData); + const patientIds = patients.map(p => p.id); + const handler = new SurescriptsSendBatchRequestHandlerDirect( + new SurescriptsSftpClient({ + logLevel: "debug", + }) + ); + await handler.sendBatchRequest({ cxId, facilityId, patientIds }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/external/surescripts/client.ts(2 hunks)packages/utils/src/surescripts/send-batch-request.ts(1 hunks)packages/utils/src/surescripts/send-facility-request.ts(1 hunks)packages/utils/src/surescripts/send-patient-request.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/utils/src/surescripts/send-facility-request.ts
- packages/core/src/external/surescripts/client.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Don’t use null inside the app, only on code interacting with external interfaces/services, like DB and HTTP; convert to undefined before sending inwards into the code
Useconstwhenever possible
Useasync/awaitinstead of.then()
Naming: classes, enums: PascalCase
Naming: constants, variables, functions: camelCase
Naming: file names: kebab-case
Naming: Don’t use negative names, likenotEnabled, preferisDisabled
If possible, use decomposing objects for function parameters
Prefer Nullish Coalesce (??) than the OR operator (||) when you want to provide a default value
Avoid creating arrow functions
Use truthy syntax instead ofin- i.e.,if (data.link)notif ('link' in data)
While handling errors, keep the stack trace around: if you create a new Error (e.g., MetriportError), make sure to pass the original error as the new one’s cause so the stack trace is available upstream.
max column length is 100 chars
multi-line comments use/** */
top-level comments go after the import (save pre-import to basic file header, like license)
move literals to constants declared after imports when possible
Files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursorrules)
Use types whenever possible
Files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
**/*.ts
⚙️ CodeRabbit Configuration File
**/*.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
- U...
Files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: keshavsaharia
PR: metriport/metriport#4075
File: packages/core/src/external/surescripts/fhir/coverage.ts:18-38
Timestamp: 2025-06-25T18:45:28.839Z
Learning: In the Surescripts FHIR bundle generation, deduplication of insurance organizations is handled at the bundle-entry level in the getCoverageResources function using the deduplicateByKey function, rather than in individual resource creation functions like getInsuranceOrganization. The coverageOrganization property in SurescriptsContext is used to track deduplicated organizations.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.
Learnt from: keshavsaharia
PR: metriport/metriport#4045
File: packages/core/src/external/surescripts/fhir/coverage.ts:0-0
Timestamp: 2025-06-18T18:34:10.489Z
Learning: Coverage resources in Surescripts FHIR conversion are currently excluded from bundles to prevent skewing data lift metrics. The team plans to examine available insurance data thoroughly before including properly structured Coverage resources with mandatory FHIR R4 elements like beneficiary and payor references.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: lucasdellabella
PR: metriport/metriport#3781
File: packages/core/src/fhir-deduplication/shared.ts:58-72
Timestamp: 2025-05-06T01:54:38.058Z
Learning: In the FHIR deduplication process, when merging resources in `mergeIntoTargetResource`, only the extensions from the target resource plus a reference extension to the source are intentionally preserved, discarding any extensions unique to the source resource. This maintains parity with the previous implementation's behavior.
Learnt from: thomasyopes
PR: metriport/metriport#3936
File: packages/core/src/external/ehr/athenahealth/index.ts:472-482
Timestamp: 2025-06-06T15:37:44.571Z
Learning: In AthenaHealth medication creation, even if medication statements have the same start/stop dates, they should not be deduplicated because there may be relevant differences beyond dates (dosage, administration routes, prescriber info, etc.) that make each statement unique and worth creating separately.
Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.
Learnt from: keshavsaharia
PR: metriport/metriport#4075
File: packages/core/src/external/surescripts/fhir/medication-request.ts:76-83
Timestamp: 2025-06-25T18:42:07.231Z
Learning: In packages/core/src/external/surescripts/fhir/medication-request.ts, the getDispenseNote and getDosageInstruction functions are intentionally identical because the dashboard requires both the note and dosageInstruction fields of MedicationRequest to be populated with detail.directions for proper note display functionality.
Learnt from: thomasyopes
PR: metriport/metriport#3981
File: packages/core/src/external/ehr/canvas/index.ts:650-657
Timestamp: 2025-06-11T16:56:22.035Z
Learning: In Canvas write-back (`createMedicationStatements`), the intended business logic is to submit only the most recent medication statement; older statements should be ignored.
Learnt from: leite08
PR: metriport/metriport#4043
File: packages/core/src/command/consolidated/consolidated-create.ts:0-0
Timestamp: 2025-06-18T00:18:59.144Z
Learning: In the consolidated bundle creation process (packages/core/src/command/consolidated/consolidated-create.ts), the team prefers strict failure behavior where if any component (including pharmacy resources) fails to be retrieved, the entire consolidation should fail rather than gracefully degrading. This ensures data consistency and prevents incomplete bundles.
Learnt from: keshavsaharia
PR: metriport/metriport#4058
File: packages/core/src/external/quest/fhir/bundle.ts:68-76
Timestamp: 2025-07-31T17:11:00.860Z
Learning: When reviewing code that involves transformations to FHIR resources, always search the codebase for existing FHIR utility functions (such as UUID validation, URL wrapping, reference generation, etc.) to avoid code duplication and ensure consistency with established patterns.
Learnt from: lucasdellabella
PR: metriport/metriport#4181
File: packages/api/src/command/medical/patient/batch-utils.ts:63-78
Timestamp: 2025-07-17T00:19:09.217Z
Learning: In batch processing utilities like packages/api/src/command/medical/patient/batch-utils.ts, user lucasdellabella prefers to keep the utility focused on its core responsibility (batching) and delegate retry logic to the calling code, rather than implementing retry mechanisms within the utility itself.
Learnt from: thomasyopes
PR: metriport/metriport#4039
File: packages/core/src/external/surescripts/fhir/medication.ts:0-0
Timestamp: 2025-06-19T21:51:26.432Z
Learning: In Surescripts FHIR conversion code, numeric validation for fields like detail.quantityDispensed is not considered necessary - the team prefers not to add defensive validation for NaN values in these conversions.
📚 Learning: the functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is ...
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.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in packages/api/src/routes/internal/analytics-platform/index.ts, the patientid parameter handling is...
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.581Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in packages/api/src/command/medical/patient/get-patient-facilities.ts, the patient.facilityids field...
Learnt from: leite08
PR: metriport/metriport#4194
File: packages/api/src/command/medical/patient/get-patient-facilities.ts:36-44
Timestamp: 2025-07-16T01:38:14.080Z
Learning: In packages/api/src/command/medical/patient/get-patient-facilities.ts, the patient.facilityIds field is strongly typed as array of strings (non-nullable), so null/undefined checks are not needed when accessing this property.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in packages/core/src/external/sftp/surescripts/client.ts, the standalone getpatientloadfilename func...
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/sftp/surescripts/client.ts:1-134
Timestamp: 2025-05-28T19:23:20.179Z
Learning: In packages/core/src/external/sftp/surescripts/client.ts, the standalone getPatientLoadFileName function intentionally omits the transmission ID from the filename, which differs from the class method getPatientLoadFileName. This difference in filename generation is expected behavior.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: for utility scripts in the packages/utils directory, developers are expected to update the main cons...
Learnt from: leite08
PR: metriport/metriport#3859
File: packages/utils/src/consolidated/check-patient-consolidated.ts:28-32
Timestamp: 2025-05-26T16:24:10.245Z
Learning: For utility scripts in the packages/utils directory, developers are expected to update the main constants at the top of the file (like bundleFilePath, cxId, patientId) rather than implementing command-line argument parsing or environment variable reading. This is the preferred pattern for configuring utility scripts in this codebase.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in the `getconsolidatedpatientdata` function from `@metriport/core/command/consolidated/consolidated...
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/core/src/command/consolidated/search/fhir-resource/search-semantic.ts:36-36
Timestamp: 2025-05-19T13:53:09.828Z
Learning: In the `getConsolidatedPatientData` function from `@metriport/core/command/consolidated/consolidated-get`, only the `patient` parameter is required. Other parameters like `requestId`, `resources`, `dateFrom`, `dateTo`, `fromDashboard`, and `forceDataFromFhir` are all optional.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in v0 of the surescripts implementation (packages/utils/src/surescripts/generate-patient-load-file.t...
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/utils/src/surescripts/generate-patient-load-file.ts:92-94
Timestamp: 2025-05-30T01:11:35.300Z
Learning: In V0 of the Surescripts implementation (packages/utils/src/surescripts/generate-patient-load-file.ts), there is always only 1 facility ID, so the current facility_ids parsing logic with substring operations is sufficient and additional validation is not necessary.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: the writemedicationtochart function in packages/api/src/external/ehr/athenahealth/command/write-back...
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Applied to files:
packages/utils/src/surescripts/send-patient-request.tspackages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientdata.firstna...
Learnt from: lucasdellabella
PR: metriport/metriport#4098
File: packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts:10-22
Timestamp: 2025-06-27T01:50:14.227Z
Learning: In packages/api/src/routes/medical/dtos/tcm-encounter-dto.ts, the patient fields patientData.firstName, patientData.lastName, and patientData.dob are guaranteed to be non-nullable values, so defensive null/undefined checks are not needed when accessing these fields.
Applied to files:
packages/utils/src/surescripts/send-patient-request.ts
📚 Learning: `surescriptssftpclient` falls back to environment-variable credentials when none are supplied explic...
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/utils/src/surescripts/receive-response.ts:16-19
Timestamp: 2025-06-12T05:54:31.454Z
Learning: `SurescriptsSftpClient` falls back to environment-variable credentials when none are supplied explicitly, so callers (e.g., CLI scripts) can safely instantiate it with no arguments as long as the relevant env vars are set.
Applied to files:
packages/utils/src/surescripts/send-patient-request.ts
📚 Learning: in packages/core/src/external/sftp/surescripts/client.ts, the surescriptssftpclient constructor inte...
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/sftp/surescripts/client.ts:1-134
Timestamp: 2025-05-28T19:23:20.179Z
Learning: In packages/core/src/external/sftp/surescripts/client.ts, the SurescriptsSftpClient constructor intentionally uses config.publicKey as the SFTP password field and config.username as the SFTP username field. This is the expected configuration for Surescripts SFTP authentication.
Applied to files:
packages/utils/src/surescripts/send-patient-request.ts
📚 Learning: in batch processing utilities like packages/api/src/command/medical/patient/batch-utils.ts, user luc...
Learnt from: lucasdellabella
PR: metriport/metriport#4181
File: packages/api/src/command/medical/patient/batch-utils.ts:63-78
Timestamp: 2025-07-17T00:19:09.217Z
Learning: In batch processing utilities like packages/api/src/command/medical/patient/batch-utils.ts, user lucasdellabella prefers to keep the utility focused on its core responsibility (batching) and delegate retry logic to the calling code, rather than implementing retry mechanisms within the utility itself.
Applied to files:
packages/utils/src/surescripts/send-batch-request.ts
📚 Learning: the conversion of the get `/internal/organization` endpoint from returning a single organization to ...
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Applied to files:
packages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in the patient synchronization architecture, the flow follows this pattern: (1) `ehr-sync-patient-cl...
Learnt from: thomasyopes
PR: metriport/metriport#3427
File: packages/core/src/external/ehr/api/sync-patient.ts:16-55
Timestamp: 2025-03-11T20:42:46.516Z
Learning: In the patient synchronization architecture, the flow follows this pattern: (1) `ehr-sync-patient-cloud.ts` sends messages to an SQS queue, (2) the `ehr-sync-patient` Lambda consumes these messages, and (3) the Lambda uses the `syncPatient` function to make the API calls to process the patient data.
Applied to files:
packages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortidschema are acc...
Learnt from: RamilGaripov
PR: metriport/metriport#3976
File: packages/api/src/routes/medical/patient.ts:541-543
Timestamp: 2025-06-18T21:05:22.256Z
Learning: In packages/api/src/routes/medical/patient.ts, inline schema definitions like cohortIdSchema are acceptable and don't need to be moved to separate schema files when the user prefers to keep them inline.
Applied to files:
packages/utils/src/surescripts/send-batch-request.ts
📚 Learning: in the metriport codebase, larger batch sizes (e.g., 100) are preferred for production code when pro...
Learnt from: RamilGaripov
PR: metriport/metriport#3463
File: packages/api/src/command/medical/patient/settings/create-patient-settings.ts:0-0
Timestamp: 2025-03-19T04:37:53.350Z
Learning: In the Metriport codebase, larger batch sizes (e.g., 100) are preferred for production code when processing operations on multiple patients, particularly for patient settings updates. Small batch sizes (e.g., 1) may be used during local testing but should be increased before deployment to production.
Applied to files:
packages/utils/src/surescripts/send-batch-request.ts
🧬 Code Graph Analysis (1)
packages/utils/src/surescripts/send-batch-request.ts (3)
packages/core/src/external/surescripts/command/send-batch-request/send-batch-request-direct.ts (1)
SurescriptsSendBatchRequestHandlerDirect(7-17)packages/core/src/external/surescripts/client.ts (1)
SurescriptsSftpClient(25-366)packages/core/src/external/surescripts/data-mapper.ts (1)
SurescriptsDataMapper(19-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/utils/src/surescripts/send-patient-request.ts (1)
13-15: LGTM! Improved CLI option validation.Converting from
.option()to.requiredOption()eliminates the need for manual runtime validation and provides clearer user feedback when required parameters are missing.packages/utils/src/surescripts/send-batch-request.ts (1)
27-58: Good refactoring with improved type safety.The shift to destructured parameters with explicit typing improves code clarity and type safety compared to the previous
program.opts()approach.However, consider enhancing the validation logic to provide clearer error messages:
- if (patientIds) { + if (patientIds && patientIds.trim()) { // ... existing logic - } else if (csvData) { + } else if (csvData && csvData.trim()) { // ... existing logic } else { - throw new Error("Patient IDs or CSV data is required"); + throw new Error("Either --patient-ids or --csv-data must be provided"); }
Comment on lines
+17
to
+23
| .requiredOption("--cx-id <cx>", "The CX ID of the requester") | ||
| .requiredOption("--facility-id <facility>", "The facility ID of the requester") | ||
| .requiredOption( | ||
| "--patient-ids <patient>", | ||
| "Specific patient IDs (comma separated) for the request" | ||
| ) | ||
| .requiredOption("--csv-data <csv>", "The CSV data file to use for patient load") |
There was a problem hiding this comment.
Fix the conflicting option requirements.
Making both --patient-ids and --csv-data required creates a logical inconsistency since the action handler treats them as mutually exclusive alternatives (lines 39-56). Users will be forced to provide both options even when they only need one approach.
Consider using option groups or making them optional with validation in the action handler:
- .requiredOption("--cx-id <cx>", "The CX ID of the requester")
- .requiredOption("--facility-id <facility>", "The facility ID of the requester")
- .requiredOption(
- "--patient-ids <patient>",
- "Specific patient IDs (comma separated) for the request"
- )
- .requiredOption("--csv-data <csv>", "The CSV data file to use for patient load")
+ .requiredOption("--cx-id <cx>", "The CX ID of the requester")
+ .requiredOption("--facility-id <facility>", "The facility ID of the requester")
+ .option(
+ "--patient-ids <patient>",
+ "Specific patient IDs (comma separated) for the request"
+ )
+ .option("--csv-data <csv>", "The CSV data file to use for patient load")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .requiredOption("--cx-id <cx>", "The CX ID of the requester") | |
| .requiredOption("--facility-id <facility>", "The facility ID of the requester") | |
| .requiredOption( | |
| "--patient-ids <patient>", | |
| "Specific patient IDs (comma separated) for the request" | |
| ) | |
| .requiredOption("--csv-data <csv>", "The CSV data file to use for patient load") | |
| .requiredOption("--cx-id <cx>", "The CX ID of the requester") | |
| .requiredOption("--facility-id <facility>", "The facility ID of the requester") | |
| .option( | |
| "--patient-ids <patient>", | |
| "Specific patient IDs (comma separated) for the request" | |
| ) | |
| .option("--csv-data <csv>", "The CSV data file to use for patient load") |
🤖 Prompt for AI Agents
In packages/utils/src/surescripts/send-batch-request.ts between lines 17 and 23,
the options --patient-ids and --csv-data are both marked as required, but they
are mutually exclusive in the action handler logic. To fix this, remove the
required flag from both options and instead implement validation inside the
action handler to ensure that exactly one of these options is provided by the
user. This will prevent forcing users to provide both options while maintaining
correct input validation.
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
lucasdellabella
approved these changes
Aug 3, 2025
metriport/metriport-internal#1040 Signed-off-by: Keshav Saharia <keshav@metriport.com>
1 task
This was referenced Aug 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues:
Dependencies
Description
Adds deduplication of
MedicationDispenseresources to the deduplication algorithm, and ensures that the reference is accurately maintained to both theMedicationandMedicationRequest.For Surescripts, removed the temporary global context for deduplication, and instead generate a new set of resources for each row to rely on the final deduplication algorithm to reduce them into a final conversion bundle. In the future, this final step of deduplication can be removed to ensure that resources are only deduplicated at the final stage of the data pipeline.
Testing
There is an internally-available sample dataset that can be used to test the one-to-one validity of our Surescripts data against an independent alternate PBM data source - see the Surescripts notion page.
npm run surescripts -- dr-firstfor analysis and validationRelease Plan
TODO
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores