Eng 513 cw v2 migration 3 #4470
Merged
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
…a updates Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
WalkthroughAdds multi-version CommonWell (CW) support: introduces CW v2 types/flows alongside CW v1, version-aware unlink/downgrade logic with retries, centralized v1/v2 document dispatch and converters, OID parsing utilities, and widespread import/path reorganizations. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant API as unlinkPatientFromOrganization
participant DB as InvalidLinks DB
participant CWV1 as CommonWell v1
participant CWV2 as CommonWell v2
participant SVC as OpenSearch / Documents
Caller->>API: unlink({ patientId, orgId, oid, ... })
API->>DB: persist invalidation intent (createOrUpdateInvalidLinks)
API->>SVC: schedule document/index invalidation
par Versioned unlink
alt link is CW v2
API->>CWV2: attempt Unlink (use link.Links?.Unlink href)
CWV2-->>API: success / error
opt missing/unimplemented
API->>CWV1: fallback v1-style downgrade/unlink
CWV1-->>API: success / error
end
else link is CW v1
API->>CWV1: perform v1 downgrade/unlink (provider ref / parsed OID)
CWV1-->>API: success / error
end
and
API->>DB: updateCWPatientData (version-aware updates)
end
API-->>Caller: aggregated result after Promise.allSettled
sequenceDiagram
autonumber
actor Client
participant API as queryAndProcessDocuments
participant Flag as FeatureFlagService
participant V1 as CW v1 Processor
participant V2 as CW v2 Processor
Client->>API: queryAndProcessDocuments(opts)
API->>Flag: isCommonwellV2EnabledForCx(cxId)
alt V2 enabled
API->>V2: queryAndProcessDocumentsV2(opts)
V2-->>API: complete / errors
else
API->>V1: queryAndProcessDocumentsV1(opts)
V1-->>API: complete / errors
end
API-->>Client: void / 200
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
…e methods, upd imports Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
… fixes Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
4 tasks
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Ref: #000 - @metriport/commonwell-sdk@6.2.4-alpha.0 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Ref: #000 - @metriport/commonwell-sdk@6.2.4-alpha.1 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
238-249: Bug on retries: listeners aren’t reattached after stream reset.If a retry happens,
writeStreamis replaced butonDataFn/onEndFnaren’t rebound, breaking accumulation and logging.Apply:
resetStream: () => { const resp = setOrResetStream(); writeStream = resp.writeStream; downloadIntoS3 = resp.promise; + // Re-attach listeners for the new stream instance + if (onDataFn) writeStream.on("data", onDataFn); + if (onEndFn) writeStream.on("end", onEndFn); },
♻️ Duplicate comments (2)
packages/api/src/external/commonwell-v1/document/document-query.ts (1)
20-20: LGTM: CloudWatch metric import and unit usage in this file are correct.The added
reportMetricimport pairs with the call that usesunit: "Milliseconds", resolving the earlier contract issue.packages/commonwell-sdk/src/client/commonwell.ts (1)
644-660: Return a concrete Error from getDescriptiveError (follow-up on prior review).Currently returns unknown; callers throw the result. Return Error and always wrap non-axios errors with MetriportError and original cause.
- private getDescriptiveError(error: unknown, title: string): unknown { + private getDescriptiveError(error: unknown, title: string): Error { if (isAxiosError(error)) { const status = error.response?.status; const data = error.response?.data; const responseBody = data ? JSON.stringify(data) : undefined; const cwReference = this.lastTransactionId; if (status === httpStatus.BAD_REQUEST) { return new BadRequestError(title, error, { status, cwReference, responseBody }); } if (status === httpStatus.NOT_FOUND) { return new NotFoundError(title, error, { status, cwReference, responseBody }); } return new MetriportError(title, error, { status, cwReference, responseBody }); } - return error; + const cause = error instanceof Error ? error : new Error(String(error)); + return new MetriportError(title, cause); }
🧹 Nitpick comments (12)
packages/api/src/external/commonwell-v1/document/document-query.ts (3)
32-33: Minor: Disambiguate similarly named “document-query-storage-info” imports.Two modules with the same basename can be confusing. Consider import aliases to make intent obvious at call sites (e.g.,
getS3SignedUrl,StorageInfo,getS3InfoForCommonwellV1). This is a readability-only change; no behavior impact.Also applies to: 36-41, 57-61
306-314: Emit the duration metric in finally and trim heavy debug log.Currently the metric won’t fire on errors, and
JSON.stringify(queryResponse)can flood logs. Suggest:
- Always report duration in
finally.- Log only the entry count.
Apply:
function reportDocQueryMetric(queryStart: number) { const queryDuration = Date.now() - queryStart; reportMetric({ name: context, value: queryDuration, unit: "Milliseconds", additionalDimension: "CommonWell", }); } @@ const queryStart = Date.now(); try { const queryResponse = await commonWell.queryDocumentsFull(queryMeta, cwData.patientId); - reportDocQueryMetric(queryStart); - debug(`resp queryDocumentsFull: ${JSON.stringify(queryResponse)}`); + debug(`resp queryDocumentsFull: entries=${queryResponse.entry?.length ?? 0}`); @@ - } catch (error) { + } catch (error) { throw new CommonwellError("Error querying documents from CommonWell", error, { cwReference: commonWell.lastReferenceHeader, context, }); - } + } finally { + reportDocQueryMetric(queryStart); + }If you adopt this here, mirror in the v2 file to keep parity.
Also applies to: 320-325, 372-378
350-365: Nit: Avoid potential overwrite in content spread.Place the spread first and the explicit
locationlast to ensure the intended value wins even ifd.content.locationis mutated upstream.- content: { location: d.content.location, ...d.content }, + content: { ...d.content, location: d.content.location },packages/commonwell-sdk/src/client/commonwell.ts (6)
147-151: Tighten types: postRequest should accept undefined; axiosErrorResponse should return never.The error interceptor can call postRequest with an undefined response; current types rely on any. Also, the interceptor always throws, so return type should be never.
- private postRequest(response: AxiosResponse): void { + private postRequest(response?: AxiosResponse): void { this._lastTransactionId = response && response.headers ? response.headers["x-trace-id"] : undefined; } - private axiosErrorResponse(_this: CommonWell) { - //eslint-disable-next-line @typescript-eslint/no-explicit-any - return (error: any): AxiosResponse => { - _this && _this.postRequest(error.response); - throw error; - }; - } + private axiosErrorResponse(_this: CommonWell) { + return (error: unknown): never => { + // best-effort capture of trace id if we have an axios-like error + _this && _this.postRequest((error as { response?: AxiosResponse })?.response); + throw error; + }; + }Also applies to: 158-164
178-195: Parse with the concrete create/update response schema to avoid shape drift.You build PatientCreateOrUpdateResp manually from patientResponseSchema. Prefer validating the final object with the dedicated schema.
- const parsed = patientResponseSchema.parse(resp.data); - const links = parsed.Patients[0].Links; - return { - Links: links, - status: parsed.status ?? undefined, - }; + const parsed = patientResponseSchema.parse(resp.data); + const result = { + Links: parsed.Patients[0].Links, + status: parsed.status ?? undefined, + }; + // Validate the returned shape against the intended schema + return patientCreateOrUpdateRespSchema.parse(result);Add import (outside this range):
import { patientCreateOrUpdateRespSchema } from "../models/patient";
241-244: Fix typo and clarify error: enforce non-empty string id.- if (!idOrParams) { - throw new Error("Programming error, 'id' is required when providing separated parametrs"); - } + if (idOrParams.trim().length === 0) { + throw new MetriportError("Programming error: 'id' must be a non-empty string"); + }
421-423: Prefer axios params over manual querystring concatenation.Let axios handle encoding and keep headers/params separate.
- const resp = await this.executeWithRetriesOn500IfEnabled(() => - this.api.get(url + `?${params.toString()}`, { headers }) - ); + const resp = await this.executeWithRetriesOn500IfEnabled(() => + this.api.get(url, { headers, params }) + );
583-585: Use truthy property checks instead of the in operator.Matches project guideline and keeps the code concise.
- if (!("resourceType" in binary)) { + if (!binary?.resourceType) { throw new CommonwellError(errorMessage, undefined, { reason: "Missing resourceType" }); }
206-210: Removeundefinedfrom the firstgetPatientoverload
In packages/commonwell-sdk/src/client/commonwell.ts, update the params overload toasync getPatient( params: GetPatientParams, options?: BaseOptions ): Promise<PatientResponseItem>;so it matches the implementation (always returns or throws).
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (3)
52-57: Named callbacks: good; dropanyand remove the ESLint disable.Aligns with “avoid arrow functions.” Tighten types and normalize chunk-to-string conversion.
Apply:
- //eslint-disable-next-line @typescript-eslint/no-explicit-any - function onData(chunk: any) { - downloadedDocument += chunk; - } - function onEnd() { + function onData(chunk: Buffer | string): void { + downloadedDocument += typeof chunk === "string" ? chunk : chunk.toString("utf8"); + } + function onEnd(): void { log("Finished downloading document"); - } + }
76-82: Gate XML parsing by detected/downloaded content type (v1 parity).Prefer
downloadResult.contentTypewhen available; fall back todocument.mimeType.Apply:
- if (downloadedDocument && isMimeTypeXML(document.mimeType)) { + if (downloadedDocument && isMimeTypeXML(downloadResult.contentType ?? document.mimeType)) { return this.parseXmlFile({
106-113: Rename to camelCase for consistency.Variable names should use camelCase.
Apply:
- const old_extension = path.extname(fileInfo.name); + const oldExtension = path.extname(fileInfo.name); @@ - if (mimeType === document.mimeType || old_extension === fileExtension) { + if (mimeType === document.mimeType || oldExtension === fileExtension) { return downloadResult; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
packages/api/src/external/commonwell-v1/document/__tests__/index-to-fhir.test.ts(1 hunks)packages/api/src/external/commonwell-v1/document/document-query.ts(4 hunks)packages/api/src/external/hie/__tests__/validate-patient-links.test.ts(25 hunks)packages/api/src/external/hie/validate-patient-links.ts(2 hunks)packages/commonwell-cert-runner/src/single-commands/patient-delete.ts(2 hunks)packages/commonwell-cert-runner/src/single-commands/patient-get-links.ts(2 hunks)packages/commonwell-cert-runner/src/single-commands/patient-get.ts(2 hunks)packages/commonwell-sdk/src/client/commonwell.ts(11 hunks)packages/core/src/external/commonwell-v1/document/document-downloader-local.ts(1 hunks)packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/api/src/external/commonwell-v1/document/tests/index-to-fhir.test.ts
- packages/commonwell-cert-runner/src/single-commands/patient-delete.ts
- packages/api/src/external/hie/tests/validate-patient-links.test.ts
- packages/commonwell-cert-runner/src/single-commands/patient-get.ts
- packages/commonwell-cert-runner/src/single-commands/patient-get-links.ts
- packages/api/src/external/hie/validate-patient-links.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/core/src/external/commonwell-v2/document/document-downloader-local-v2.tspackages/core/src/external/commonwell-v1/document/document-downloader-local.tspackages/commonwell-sdk/src/client/commonwell.tspackages/api/src/external/commonwell-v1/document/document-query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.tspackages/core/src/external/commonwell-v1/document/document-downloader-local.tspackages/commonwell-sdk/src/client/commonwell.tspackages/api/src/external/commonwell-v1/document/document-query.ts
🧠 Learnings (30)
📓 Common learnings
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.tspackages/core/src/external/commonwell-v1/document/document-downloader-local.tspackages/commonwell-sdk/src/client/commonwell.tspackages/api/src/external/commonwell-v1/document/document-query.ts
📚 Learning: 2025-09-01T20:39:53.630Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:0-0
Timestamp: 2025-09-01T20:39:53.630Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain the existing string-based content accumulation approach for document downloading rather than switching to Buffer-based handling, prioritizing production stability and proven logic over technical improvements when under shipping deadlines.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.tspackages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-01T21:09:36.366Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:104-111
Timestamp: 2025-09-01T21:09:36.366Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 confirmed that the downloadedDocument variable and associated string-based file type detection approach should be maintained as-is because it has been working reliably in production with CW v1 for many months, and the code is primarily used for XML document parsing where binary corruption is not a concern.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.tspackages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-01T21:08:47.044Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:55-61
Timestamp: 2025-09-01T21:08:47.044Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, the downloadedDocument variable uses string concatenation for chunk accumulation, which is acceptable because it's only used for XML document parsing (text-based content) when isMimeTypeXML(downloadResult.contentType) returns true. For non-XML documents, the variable is not used, so binary corruption concerns don't apply in this context.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.tspackages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-02T14:30:47.451Z
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.tspackages/core/src/external/commonwell-v1/document/document-downloader-local.tspackages/api/src/external/commonwell-v1/document/document-query.ts
📚 Learning: 2025-06-23T16:50:06.930Z
Learnt from: leite08
PR: metriport/metriport#4073
File: packages/shared/src/common/env-var.ts:20-27
Timestamp: 2025-06-23T16:50:06.930Z
Learning: In TypeScript files, avoid creating arrow functions and use regular function declarations instead. For example, instead of `export const myFunction = (param: string): string => { ... }`, use `export function myFunction(param: string): string { ... }`. This applies to all exported functions in the codebase.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-06-18T04:51:00.057Z
Learnt from: leite08
PR: metriport/metriport#4044
File: packages/api/src/domain/medical/__tests__/tcm-encounter.ts:4-4
Timestamp: 2025-06-18T04:51:00.057Z
Learning: In TypeScript files, avoid using arrow functions for function declarations. Use regular function declarations instead. Arrow functions are acceptable only when passing functions as parameters to other functions or for very simple one-liners. This follows Metriport's development best practices.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-01T22:26:44.481Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts:0-0
Timestamp: 2025-09-01T22:26:44.481Z
Learning: The file packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts contains existing logic that was moved from elsewhere, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.tspackages/commonwell-sdk/src/client/commonwell.tspackages/api/src/external/commonwell-v1/document/document-query.ts
📚 Learning: 2025-05-20T21:26:26.804Z
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/commonwell-sdk/src/client/commonwell.tspackages/api/src/external/commonwell-v1/document/document-query.ts
📚 Learning: 2025-09-01T19:57:53.618Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.tspackages/api/src/external/commonwell-v1/document/document-query.ts
📚 Learning: 2025-06-27T01:50:14.227Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-06-06T16:45:31.832Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-25T17:52:19.940Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/commonwell-sdk/src/client/commonwell-member.ts:231-233
Timestamp: 2025-08-25T17:52:19.940Z
Learning: In packages/commonwell-sdk/src/client/commonwell-member.ts, the getOneOrg method currently returns undefined for all non-404 HTTP errors (instead of throwing MetriportError) as a temporary workaround while waiting for the service provider to fix issues on their end. This is tracked in TODO ENG-668 and will be reverted later once the external dependency is resolved.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T19:51:11.022Z
Learnt from: CR
PR: metriport/metriport#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-01T19:51:11.022Z
Learning: Applies to **/*.{ts,tsx} : Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-26T02:33:26.777Z
Learnt from: leite08
PR: metriport/metriport#4469
File: packages/fhir-converter/src/lib/workers/worker.js:75-87
Timestamp: 2025-08-26T02:33:26.777Z
Learning: User leite08 confirmed that the debug logging approach in packages/fhir-converter/src/lib/workers/worker.js was already reviewed and approved in the feature PR, even though it logs both error messages and stack traces.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T23:29:58.588Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:276-287
Timestamp: 2025-09-01T23:29:58.588Z
Learning: RamilGaripov confirmed that the capture.error(msg, { extra: { error } }) pattern used in the current code at lines 276-287 of packages/api/src/external/commonwell-v2/document/document-query.ts is the correct and established pattern for the Metriport codebase, and suggestions to change this pattern should not be made.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-16T00:54:56.156Z
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-27T23:12:23.978Z
Learnt from: thomasyopes
PR: metriport/metriport#4478
File: packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts:44-47
Timestamp: 2025-08-27T23:12:23.978Z
Learning: In packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts, the intentional swallowing of HTTP 400 errors in startContributeBundlesJob via early return is by design, as confirmed by thomasyopes. This pattern should not be flagged in future reviews.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T21:54:32.973Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.tspackages/api/src/external/commonwell-v1/document/document-query.ts
📚 Learning: 2025-06-18T21:05:22.256Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-15T00:00:45.080Z
Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/shared/src/domain/patient.ts:5-5
Timestamp: 2025-08-15T00:00:45.080Z
Learning: The patientSchema in packages/shared/src/domain/patient.ts is used in a limited subsystem scope ("SS") and is not the same as other Patient schemas referenced throughout the codebase, making additive optional field changes like externalId safe and non-breaking.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T23:31:30.405Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient.ts:850-886
Timestamp: 2025-09-01T23:31:30.405Z
Learning: In packages/commonwell-sdk/src/models/patient.ts, the Patient schema defines name as z.array(humanNameSchema), address as z.array(addressSchema), and identifier as z.array(identifierSchema) without .optional() or .nullish(), which means these fields are always present as arrays and will never be undefined. However, telecom is defined with .nullish() so it can be undefined and requires optional chaining when accessing it in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-16T18:51:08.416Z
Learnt from: leite08
PR: metriport/metriport#4095
File: packages/commonwell-cert-runner/src/single-commands/patient-create.ts:35-36
Timestamp: 2025-07-16T18:51:08.416Z
Learning: In packages/commonwell-cert-runner and the CommonWell SDK, PatientCollection.Patients is strongly typed as an array (z.array(patientCollectionItemSchema)), so resp.Patients will always exist and be an array. The getMetriportPatientIdOrFail utility function is designed to handle undefined/null input with proper error messages, making additional bounds checking unnecessary.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:15:56.485Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:24-34
Timestamp: 2025-09-01T22:15:56.485Z
Learning: In packages/api/src/external/commonwell-v2/patient/patient-demographics.ts, the NetworkLink.Patient.address field is guaranteed to be defined as it's a required array field in the CommonWell v2 Patient schema, so optional chaining is not needed when accessing this property.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-18T09:33:29.581Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-06-03T21:02:23.374Z
Learnt from: leite08
PR: metriport/metriport#3953
File: packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts:28-30
Timestamp: 2025-06-03T21:02:23.374Z
Learning: The `makePatient()` function returns `PatientWithId` type which requires the `id` field to be present, ensuring `patient.id` is never undefined.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:06:31.369Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-conversion.ts:60-71
Timestamp: 2025-09-01T22:06:31.369Z
Learning: In packages/api/src/external/commonwell-v2/patient/patient-conversion.ts, the patient.data.address field is guaranteed to be defined as it's a required array field in the Patient schema, so optional chaining is not needed when accessing this property.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T20:04:05.083Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts:81-91
Timestamp: 2025-09-01T20:04:05.083Z
Learning: In CommonWell v2 document conversion (packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts), the doc.status field can be used directly without additional validation because the underlying CommonWell SDK v2 already provides schema validation that ensures DocumentReference.status values conform to FHIR R4 requirements ("current" | "superseded" | "entered-in-error").
Applied to files:
packages/api/src/external/commonwell-v1/document/document-query.ts
📚 Learning: 2025-09-01T20:04:05.083Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts:81-91
Timestamp: 2025-09-01T20:04:05.083Z
Learning: In CommonWell v2 document conversion (packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts), the doc.status field can be used directly without additional validation because the underlying CommonWell SDK v2 document schema already validates that status values conform to FHIR R4 DocumentReference.status requirements ("current" | "superseded" | "entered-in-error").
Applied to files:
packages/api/src/external/commonwell-v1/document/document-query.ts
🧬 Code graph analysis (3)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
packages/core/src/util/log.ts (1)
log(10-23)
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts (1)
packages/core/src/util/log.ts (1)
log(10-23)
packages/commonwell-sdk/src/client/commonwell.ts (5)
packages/commonwell-sdk/src/models/patient.ts (8)
Patient(43-43)PatientCreateOrUpdateResp(72-72)patientResponseSchema(60-63)PatientResponseItem(58-58)PatientExistingLinks(92-92)patientExistingLinksSchema(88-91)PatientProbableLinks(112-112)patientProbableLinksRespSchema(108-111)packages/commonwell-sdk/src/client/commonwell-api.ts (1)
GetPatientParams(47-54)packages/core/src/mpi/normalize-patient.ts (1)
normalizePatient(19-55)packages/commonwell-sdk/src/common/util.ts (2)
encodeCwPatientId(61-71)encodePatientIdForDocumentExchange(97-111)packages/shared/src/index.ts (3)
MetriportError(43-43)BadRequestError(42-42)NotFoundError(44-44)
⏰ 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). (7)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/api/src/external/commonwell-v1/document/document-query.ts (1)
294-301: No action needed: internalGetDocuments is file-private with no external imports
Confirmed there are no export statements or cross-file imports referencing internalGetDocuments in either v1 or v2 document-query files.packages/commonwell-sdk/src/client/commonwell.ts (1)
507-512: Good guard on subjectId derivation.Failing fast on an invalid/undecodable CX patientId prevents downstream 404 fanouts. LGTM.
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts (1)
60-65: Callback functions consistently converted to named declarations.The change from arrow functions to named function declarations aligns with the coding guidelines that state "Avoid creating arrow functions." This maintains consistency with the v2 implementation and follows the established pattern for callback functions in this codebase.
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
106-113: fileExtension always includes a leading dot detectFileType returns fileExtension prefixed with '.', matching path.extname semantics; no normalization required.Likely an incorrect or invalid review comment.
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/commonwell-sdk/src/client/commonwell.ts (1)
644-660: Return a concrete Error from getDescriptiveError (repeat of prior feedback).Returning unknown complicates usage and typings; return Error.
Apply this diff:
- private getDescriptiveError(error: unknown, title: string): unknown { + private getDescriptiveError(error: unknown, title: string): Error { if (isAxiosError(error)) { const status = error.response?.status; const data = error.response?.data; const responseBody = data ? JSON.stringify(data) : undefined; const cwReference = this.lastTransactionId; if (status === httpStatus.BAD_REQUEST) { return new BadRequestError(title, error, { status, cwReference, responseBody }); } if (status === httpStatus.NOT_FOUND) { return new NotFoundError(title, error, { status, cwReference, responseBody }); } return new MetriportError(title, error, { status, cwReference, responseBody }); } - return error; + return error instanceof Error ? error : new MetriportError(title); }
🧹 Nitpick comments (12)
packages/core/src/external/commonwell/document/document-downloader.ts (1)
31-39: Optional: extract a named input type + TSDoc to reduce churn and clarify semantics.Using a single typed parameter improves readability and future-proofs renames.
Apply within this hunk:
- abstract download({ - sourceDocument, - destinationFileInfo, - cxId, - }: { - sourceDocument: Document; - destinationFileInfo: FileInfo; - cxId: string; - }): Promise<DownloadResult>; + /** Downloads sourceDocument into destinationFileInfo for the given connection (cxId). */ + abstract download(input: DownloadInput): Promise<DownloadResult>;Add after existing type exports (outside this hunk):
export type DownloadInput = { sourceDocument: Document; destinationFileInfo: FileInfo; cxId: string; };packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (6)
51-56: Avoid any; normalize chunk typing and drop ESLint disable.Keep behavior while adding types.
Apply:
- //eslint-disable-next-line @typescript-eslint/no-explicit-any - function onData(chunk: any) { - downloadedDocument += chunk; - } + function onData(chunk: unknown) { + downloadedDocument += Buffer.isBuffer(chunk) ? chunk.toString("utf8") : String(chunk); + }
67-71: Param rename passthrough is fine; optional readability nit.Consider aligning names in the call to match the new outer names for quicker grepping during on-call.
- downloadResult = await this.checkAndUpdateMimeType({ - document: sourceDocument, - fileInfo: destinationFileInfo, + downloadResult = await this.checkAndUpdateMimeType({ + document: sourceDocument, + fileInfo: destinationFileInfo,(Only a note: you already map correctly; no change required.)
111-118: CamelCase local var.Follow code style.
- const old_extension = path.extname(fileInfo.name); + const oldExtension = path.extname(fileInfo.name); @@ - if (mimeType === document.mimeType || old_extension === fileExtension) { + if (mimeType === document.mimeType || oldExtension === fileExtension) {
187-189: Tighten log payload.Avoid stringifying the full
requestedFileInfo; it can get noisy.- log(`No b64 found in xml. File info: ${JSON.stringify(requestedFileInfo)}`); + log(`No b64 found in xml. File: ${requestedFileInfo.name}`);
193-197: Typo in comment.s/Alternativelly/Alternatively/.
- // Alternativelly we can use the provided mediaType and calculate the extension from it + // Alternatively we can use the provided mediaType and calculate the extension from it
289-292: Prefer nullish coalescing for ContentType.Cleaner and avoids falsey pitfalls.
- ContentType: contentType ? contentType : "text/xml", + ContentType: contentType ?? "text/xml",packages/api/src/external/commonwell-v2/mock/api-mock.ts (2)
68-70: Clarify getPatient overload semantics and tweak the error message.Params overload encodes the ID; string overload assumes pre-encoded ID. Consider clarifying the error to reflect the overload in use.
- throw new Error("Programming error: 'id' is required when providing separate parameters"); + throw new Error("Programming error: 'id' is required when using getPatient(id, options).")Also applies to: 73-86
116-121: Use distinct entries to better simulate multiple links.Returning two identical entries can hide consumer bugs. Make the second entry unique.
- createProbablePatient(this.oid, this.orgName, patientId), - createProbablePatient(this.oid, this.orgName, patientId), + createProbablePatient(this.oid, this.orgName, patientId), + createProbablePatient(this.oid, this.orgName, nanoid()),packages/commonwell-sdk/src/models/patient.ts (2)
35-39: Remove stale comment (references removed Patient.link; typo “validatte”).This block is misleading now that the field is gone.
Apply this diff:
- /** - * Links to a Patient or RelatedPerson resource that concerns the same actual individual. - * The patient resource containing this link is the non-surviving patient. - * Disabled because couldn't validatte this in pre-production. - */
114-115: Move CwLinkV2 out of models in a follow-upAs the TODO notes, the version tag is API-layer specific; keep models pure.
packages/commonwell-sdk/src/client/commonwell.ts (1)
241-243: Use MetriportError for consistencyPrefer consistent error type across the client.
Apply this diff:
- throw new Error("Programming error: 'id' is required when providing separate parameters"); + throw new MetriportError("Missing patient id when using string identifier");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/api/src/external/commonwell-v2/mock/api-mock.ts(7 hunks)packages/commonwell-sdk/src/client/commonwell.ts(10 hunks)packages/commonwell-sdk/src/models/patient.ts(1 hunks)packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts(2 hunks)packages/core/src/external/commonwell/document/document-downloader.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/core/src/external/commonwell/document/document-downloader.tspackages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.tspackages/commonwell-sdk/src/models/patient.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/core/src/external/commonwell/document/document-downloader.tspackages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.tspackages/commonwell-sdk/src/models/patient.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
🧠 Learnings (32)
📓 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.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/core/src/external/commonwell/document/document-downloader.tspackages/commonwell-sdk/src/client/commonwell.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-01T21:09:36.366Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:104-111
Timestamp: 2025-09-01T21:09:36.366Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 confirmed that the downloadedDocument variable and associated string-based file type detection approach should be maintained as-is because it has been working reliably in production with CW v1 for many months, and the code is primarily used for XML document parsing where binary corruption is not a concern.
Applied to files:
packages/core/src/external/commonwell/document/document-downloader.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-01T20:39:53.630Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:0-0
Timestamp: 2025-09-01T20:39:53.630Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain the existing string-based content accumulation approach for document downloading rather than switching to Buffer-based handling, prioritizing production stability and proven logic over technical improvements when under shipping deadlines.
Applied to files:
packages/core/src/external/commonwell/document/document-downloader.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-01T21:08:47.044Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:55-61
Timestamp: 2025-09-01T21:08:47.044Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, the downloadedDocument variable uses string concatenation for chunk accumulation, which is acceptable because it's only used for XML document parsing (text-based content) when isMimeTypeXML(downloadResult.contentType) returns true. For non-XML documents, the variable is not used, so binary corruption concerns don't apply in this context.
Applied to files:
packages/core/src/external/commonwell/document/document-downloader.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-02T14:30:47.451Z
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Applied to files:
packages/core/src/external/commonwell/document/document-downloader.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-01T22:26:44.481Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts:0-0
Timestamp: 2025-09-01T22:26:44.481Z
Learning: The file packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts contains existing logic that was moved from elsewhere, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/core/src/external/commonwell/document/document-downloader.tspackages/commonwell-sdk/src/client/commonwell.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-05-20T21:26:26.804Z
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/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-08-15T00:00:45.080Z
Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/shared/src/domain/patient.ts:5-5
Timestamp: 2025-08-15T00:00:45.080Z
Learning: The patientSchema in packages/shared/src/domain/patient.ts is used in a limited subsystem scope ("SS") and is not the same as other Patient schemas referenced throughout the codebase, making additive optional field changes like externalId safe and non-breaking.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-06-27T01:50:14.227Z
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/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T19:57:53.618Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-06-06T16:45:31.832Z
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/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-16T18:51:08.416Z
Learnt from: leite08
PR: metriport/metriport#4095
File: packages/commonwell-cert-runner/src/single-commands/patient-create.ts:35-36
Timestamp: 2025-07-16T18:51:08.416Z
Learning: In packages/commonwell-cert-runner and the CommonWell SDK, PatientCollection.Patients is strongly typed as an array (z.array(patientCollectionItemSchema)), so resp.Patients will always exist and be an array. The getMetriportPatientIdOrFail utility function is designed to handle undefined/null input with proper error messages, making additional bounds checking unnecessary.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T23:31:30.405Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient.ts:850-886
Timestamp: 2025-09-01T23:31:30.405Z
Learning: In packages/commonwell-sdk/src/models/demographics.ts, the Demographics schema (which Patient extends) defines identifier as z.array(patientIdentifierSchema).min(1), name as z.array(humanNameSchema).min(1), and address as z.array(addressSchema).min(1), which guarantees these fields are always present as non-empty arrays and will never be undefined. However, telecom is defined as z.array(contactSchema).nullish() so it can be undefined and requires optional chaining when accessing it in CommonWell v2 patient data structures.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-06-03T21:02:23.374Z
Learnt from: leite08
PR: metriport/metriport#3953
File: packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts:28-30
Timestamp: 2025-06-03T21:02:23.374Z
Learning: The `makePatient()` function returns `PatientWithId` type which requires the `id` field to be present, ensuring `patient.id` is never undefined.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:15:56.485Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:24-34
Timestamp: 2025-09-01T22:15:56.485Z
Learning: In packages/api/src/external/commonwell-v2/patient/patient-demographics.ts, the NetworkLink.Patient.address field is guaranteed to be defined as it's a required array field in the CommonWell v2 Patient schema, so optional chaining is not needed when accessing this property.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/client/commonwell.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-09-01T22:16:42.635Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.635Z
Learning: In packages/commonwell-sdk/src/models/demographics.ts, the demographicsSchema defines the name field as z.array(humanNameSchema) without .optional() or .nullish(), which means patient.name is always present as an array and will never be undefined. This applies to all Patient types in CommonWell SDK including those used in PatientExistingLink and PatientProbableLink. Defensive programming with optional chaining is not needed when accessing patient.name in CommonWell v2 patient data structures.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-09-01T22:15:56.485Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:24-34
Timestamp: 2025-09-01T22:15:56.485Z
Learning: In packages/api/src/external/commonwell-v2/patient/patient-demographics.ts, the NetworkLink.Patient.address field is guaranteed to be defined as a required non-empty array (z.array(addressSchema).min(1)) in the CommonWell v2 Patient schema, so optional chaining is not needed when accessing this property.
Applied to files:
packages/api/src/external/commonwell-v2/mock/api-mock.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-08-25T17:52:19.940Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/commonwell-sdk/src/client/commonwell-member.ts:231-233
Timestamp: 2025-08-25T17:52:19.940Z
Learning: In packages/commonwell-sdk/src/client/commonwell-member.ts, the getOneOrg method currently returns undefined for all non-404 HTTP errors (instead of throwing MetriportError) as a temporary workaround while waiting for the service provider to fix issues on their end. This is tracked in TODO ENG-668 and will be reverted later once the external dependency is resolved.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T23:31:30.405Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient.ts:850-886
Timestamp: 2025-09-01T23:31:30.405Z
Learning: In packages/commonwell-sdk/src/models/patient.ts, the Patient schema defines name as z.array(humanNameSchema), address as z.array(addressSchema), and identifier as z.array(identifierSchema) without .optional() or .nullish(), which means these fields are always present as arrays and will never be undefined. However, telecom is defined with .nullish() so it can be undefined and requires optional chaining when accessing it in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-09-01T19:51:11.022Z
Learnt from: CR
PR: metriport/metriport#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-01T19:51:11.022Z
Learning: Applies to **/*.{ts,tsx} : Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-26T02:33:26.777Z
Learnt from: leite08
PR: metriport/metriport#4469
File: packages/fhir-converter/src/lib/workers/worker.js:75-87
Timestamp: 2025-08-26T02:33:26.777Z
Learning: User leite08 confirmed that the debug logging approach in packages/fhir-converter/src/lib/workers/worker.js was already reviewed and approved in the feature PR, even though it logs both error messages and stack traces.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T23:29:58.588Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:276-287
Timestamp: 2025-09-01T23:29:58.588Z
Learning: RamilGaripov confirmed that the capture.error(msg, { extra: { error } }) pattern used in the current code at lines 276-287 of packages/api/src/external/commonwell-v2/document/document-query.ts is the correct and established pattern for the Metriport codebase, and suggestions to change this pattern should not be made.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-16T00:54:56.156Z
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-27T23:12:23.978Z
Learnt from: thomasyopes
PR: metriport/metriport#4478
File: packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts:44-47
Timestamp: 2025-08-27T23:12:23.978Z
Learning: In packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts, the intentional swallowing of HTTP 400 errors in startContributeBundlesJob via early return is by design, as confirmed by thomasyopes. This pattern should not be flagged in future reviews.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T21:54:32.973Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.tspackages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-06-18T21:05:22.256Z
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/commonwell-sdk/src/client/commonwell.tspackages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-07-18T09:33:29.581Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:06:31.369Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-conversion.ts:60-71
Timestamp: 2025-09-01T22:06:31.369Z
Learning: In packages/api/src/external/commonwell-v2/patient/patient-conversion.ts, the patient.data.address field is guaranteed to be defined as it's a required array field in the Patient schema, so optional chaining is not needed when accessing this property.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:16:42.635Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.635Z
Learning: In packages/commonwell-sdk/src/models/patient.ts, the Patient schema defines the name field as z.array(humanNameSchema) without .optional() or .nullish(), which means patient.name is always present as an array and will never be undefined. Defensive programming with optional chaining is not needed when accessing patient.name in CommonWell v2 patient data structures like PatientExistingLink and PatientProbableLink.
Applied to files:
packages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-09-01T22:16:42.636Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.636Z
Learning: In packages/commonwell-sdk/src/models/demographics.ts and human-name.ts, the schema guarantees that patient.name is z.array(humanNameSchema).min(1) (always an array with at least 1 element), and within each name object, both family and given are z.array(z.string()) (always arrays, never undefined). This means defensive programming with optional chaining or null checks is unnecessary when accessing patient.name, name.family, or name.given in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/models/patient.ts
📚 Learning: 2025-08-14T23:29:53.606Z
Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/core/src/external/quest/file/file-generator.ts:55-57
Timestamp: 2025-08-14T23:29:53.606Z
Learning: In packages/shared/src/domain/patient.ts, the Patient schema defines address as z.array(...) without .optional(), meaning the address field is always present as an array. Optional chaining is not needed when accessing patient.address[0], though checking for undefined after indexing is still necessary in case the array is empty.
Applied to files:
packages/commonwell-sdk/src/models/patient.ts
🧬 Code graph analysis (4)
packages/api/src/external/commonwell-v2/mock/api-mock.ts (3)
packages/commonwell-sdk/src/models/patient.ts (5)
Patient(43-43)PatientCreateOrUpdateResp(72-72)PatientResponseItem(58-58)PatientExistingLinks(92-92)PatientProbableLinks(112-112)packages/api/src/external/commonwell-v2/mock/payloads.ts (2)
createPatientCollectionItem(54-68)createProbablePatient(70-84)packages/commonwell-sdk/src/client/commonwell-api.ts (1)
GetPatientParams(47-54)
packages/commonwell-sdk/src/client/commonwell.ts (4)
packages/commonwell-sdk/src/models/patient.ts (8)
Patient(43-43)PatientCreateOrUpdateResp(72-72)patientResponseSchema(60-63)PatientResponseItem(58-58)PatientExistingLinks(92-92)patientExistingLinksSchema(88-91)PatientProbableLinks(112-112)patientProbableLinksRespSchema(108-111)packages/commonwell-sdk/src/client/commonwell-api.ts (2)
BaseOptions(13-15)GetPatientParams(47-54)packages/commonwell-sdk/src/common/util.ts (2)
encodeCwPatientId(61-71)encodePatientIdForDocumentExchange(97-111)packages/shared/src/index.ts (3)
MetriportError(43-43)BadRequestError(42-42)NotFoundError(44-44)
packages/commonwell-sdk/src/models/patient.ts (2)
packages/commonwell-sdk/src/models/demographics.ts (1)
demographicsSchema(27-34)packages/commonwell-sdk/src/models/patient-organization.ts (1)
managingOrganizationSchema(4-7)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (3)
packages/core/src/external/commonwell/document/document-downloader.ts (3)
Document(9-13)FileInfo(15-18)DownloadResult(20-26)packages/core/src/util/log.ts (2)
out(30-35)log(10-23)packages/core/src/util/mime.ts (1)
isMimeTypeXML(11-13)
⏰ 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). (7)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (24)
packages/core/src/external/commonwell/document/document-downloader.ts (1)
31-39: Rename applied safely: Verified no remainingdownload({ document, fileInfo })usages in overrides or call sites; all were updated to{ sourceDocument, destinationFileInfo }.packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
43-48: All download call sites updated
No remaining uses of the old{document, fileInfo}shape.packages/api/src/external/commonwell-v2/mock/api-mock.ts (5)
6-11: API v2 type imports look correct.Importing GenderCodes and the v2 response/item types aligns this mock with the updated SDK models.
22-22: Good switch to encodeCwPatientId.Using encodeCwPatientId for the params-based overload matches the SDK’s expected ID formatting.
50-53: createOrUpdatePatient response migration LGTM.Returning PatientCreateOrUpdateResp with Links from createPatientCollectionItem is consistent with the new v2 shape.
Also applies to: 57-57
132-142: Probable links response looks consistent with v2 types.Returning PatientProbableLinks with two items is fine for the mock.
156-160: GenderCodes usage is correct for demographics search.Typing gender as GenderCodes and returning PatientProbableLinks aligns with the SDK.
packages/commonwell-sdk/src/models/patient.ts (10)
27-27: LGTM: exported PatientLinks type aliasPublic alias improves SDK ergonomics.
47-51: LGTM: status response wrapperConsistent, reusable envelope for endpoints returning only status.
54-59: LGTM: PatientResponseItem with required LinksMaking Links required (with Patient optional) matches the new CW v2 pattern.
60-65: Confirm “nonempty” contract for PatientsParsing enforces at least one item; ensure the API never returns an empty array for these routes, otherwise this will hard-fail parse.
68-73: LGTM: create/update response shape (Links + optional status)Also adheres to “no nulls internally” by emitting undefined.
76-81: LGTM: Existing-links Links schemaRequired Self/Unlink is reasonable given operation expectations.
82-86: LGTM: Existing-links itemStrongly typed Patient + operation Links.
88-93: LGTM: Existing-links responseAllowing empty arrays is appropriate here.
102-107: LGTM: Probable-links itemMatches CW “probable” operations surface.
108-113: LGTM: Probable-links responseAllows zero probable matches.
packages/commonwell-sdk/src/client/commonwell.ts (7)
2-9: LGTM: imports updated for richer errors, new models, and utilitiesAligns client with v2 schemas, gender typing, and ID encoders.
Also applies to: 17-23, 30-40
178-195: LGTM: createOrUpdatePatient now returns Links + statusParsing via PatientResponse and returning undefined-free status matches internal “no nulls” guideline.
250-254: LGTM: guard against multiple patientsClear error with count and id in additionalInfo.
334-341: LGTM: existing links with descriptive error mappingParse-then-throw pattern with getDescriptiveError is solid.
367-374: LGTM: probable links with descriptive error mappingConsistent with existing-links method.
408-425: LGTM: demographics-based probable linksGenderCodes typing + URLSearchParams usage looks correct.
507-512: LGTM: subject derivation for document queriesUsing encodePatientIdForDocumentExchange() and failing fast if undefined is correct.
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
Outdated
Show resolved
Hide resolved
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/api/src/external/commonwell-v2/document/document-query.ts (4)
138-149: Progress can remain “processing” on early return (keep tracked for follow-up).
setDocQueryProgressis invoked before evaluatingisTriggerDQ, and an early return at Line 220 can leave progress stuck at "processing". This was previously discussed and intentionally kept aligned with v1 for now. Please ensure an issue tracks fixing this in both versions together.Also applies to: 218-221
374-379: Sanitize OperationOutcome logging (keep details in capture, not in log).Avoid JSON-dumping potentially sensitive payloads in plain logs; keep full details in
capture.error. Suggested patch (safe for a follow-up if you prefer):- const msg = `CW Document query error`; - log(`${msg} - Category: ${category}. Cause: ${JSON.stringify(errors)}`); + const msg = `CW Document query error`; + log(`${msg} - Category: ${category}. count=${errors.length}`); capture.error(msg, { extra: { ...context, errors, category }, });
436-451: Don’t attach full DocumentReference in MetriportError.additionalInfo.Replace with a minimal, non-PHI shape (tracked earlier; follow-up acceptable).
- if (!documentId) - throw new MetriportError("Document ID is required", undefined, { - extra: JSON.stringify(document), - }); + if (!documentId) { + const printable = { + id: document.masterIdentifier?.value ?? document.id, + hasUrl: Boolean(document.content?.[0]?.attachment?.url), + contentType: document.content?.[0]?.attachment?.contentType, + }; + throw new MetriportError("Document ID is required", undefined, printable); + }
635-643: PHI risk: avoid attaching full DocumentReference to capture.error.Use the minimal printable details already available.
- capture.error(msg, { - extra: { - context: `cw.downloadDocsAndUpsertFHIR.downloadFromCWAndUploadToS3`, - patientId: patient.id, - documentReference: doc, - requestId, - error: errorToString(error), - }, - }); + capture.error(msg, { + extra: { + context: `cw.downloadDocsAndUpsertFHIR.downloadFromCWAndUploadToS3`, + patientId: patient.id, + document: getDocPrintableDetails(doc), + requestId, + error: errorToString(error), + }, + });
🧹 Nitpick comments (4)
packages/api/src/external/commonwell-v2/document/document-query.ts (4)
403-411: Prefer structured logger over console.log in helper.Use
out()for consistency and single-line logs.- if (!docLocation) { - console.log( - `Doc without location, skipping - fhirDocId ${doc.id}, originalDocId ${doc.originalId}` - ); - return undefined; - } + if (!docLocation) { + const { log } = out("cw.addLocation"); + log(`Doc without location, skipping - fhirDocId ${doc.id}, originalDocId ${doc.originalId}`); + return undefined; + }
566-616: Avoid per-document initiator fetch; cache per-call.Reduces redundant I/O and jitter across a chunk.
const chunks = chunk(docsToDownload, DOC_DOWNLOAD_CHUNK_SIZE); + // cache initiator per call + let cachedInitiatorPromise: Promise<HieInitiator> | undefined; + const getInitiatorOnce = () => + (cachedInitiatorPromise ??= getCwInitiator({ id: patient.id, cxId }, facilityId)); for (const docChunk of chunks) { @@ - uploadToS3 = async () => { - const initiator = await getCwInitiator({ id: patient.id, cxId }, facilityId); + uploadToS3 = async () => { + const initiator = await getInitiatorOnce(); const newFile = triggerDownloadDocument({ doc, fileInfo, initiator, cxId, requestId, });
792-815: Name shadowing: param ‘fileInfo’ vs imported type ‘fileInfo’.This is confusing; rename the param to avoid value/type shadowing.
-async function triggerDownloadDocument({ - doc, - fileInfo, +async function triggerDownloadDocument({ + doc, + fileInfo: s3Info, @@ - fileInfo: S3Info; + fileInfo: S3Info; @@ - const adjustedFileInfo = { - name: fileInfo.fileName, - location: fileInfo.fileLocation, - }; + const adjustedFileInfo = { + name: s3Info.fileName, + location: s3Info.fileLocation, + };
319-323: Emit CW query metric on both success and error.Currently metrics fire only on success. Call it before throwing to preserve error durations.
- const queryResponse = await commonWell.queryDocumentsFull(cwData.patientId); - reportDocQueryMetric(queryStart); + const queryResponse = await commonWell.queryDocumentsFull(cwData.patientId); + reportDocQueryMetric(queryStart); @@ - } catch (error) { - throw new CommonwellError("Error querying documents from CommonWell", error, { + } catch (error) { + reportDocQueryMetric(queryStart); + throw new CommonwellError("Error querying documents from CommonWell", error, { cwReference: commonWell.lastTransactionId, context, });Also applies to: 355-360
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/api/src/external/commonwell-v2/document/document-query.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/api/src/external/commonwell-v2/document/document-query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/api/src/external/commonwell-v2/document/document-query.ts
🧠 Learnings (43)
📓 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.
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-09-01T22:26:44.481Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts:0-0
Timestamp: 2025-09-01T22:26:44.481Z
Learning: The file packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts contains existing logic that was moved from elsewhere, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T21:54:32.973Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T19:57:53.618Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-02T14:30:47.451Z
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-20T21:26:26.804Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T20:04:05.083Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts:81-91
Timestamp: 2025-09-01T20:04:05.083Z
Learning: In CommonWell v2 document conversion (packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts), the doc.status field can be used directly without additional validation because the underlying CommonWell SDK v2 already provides schema validation that ensures DocumentReference.status values conform to FHIR R4 requirements ("current" | "superseded" | "entered-in-error").
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-21T01:50:04.471Z
Learnt from: keshavsaharia
PR: metriport/metriport#4394
File: packages/core/src/external/quest/command/fhir-converter/fhir-converter-direct.ts:5-8
Timestamp: 2025-08-21T01:50:04.471Z
Learning: In packages/core/src/external/quest/command/fhir-converter/fhir-converter-direct.ts, the QuestFhirConverterCommandDirect class is intentionally implemented as a stub/TODO for ENG-864, as confirmed by keshavsaharia. The Quest FHIR conversion feature is being developed in phases - the current PR establishes the pipeline structure, and the next PR will implement the actual conversion logic.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T20:39:53.630Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:0-0
Timestamp: 2025-09-01T20:39:53.630Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain the existing string-based content accumulation approach for document downloading rather than switching to Buffer-based handling, prioritizing production stability and proven logic over technical improvements when under shipping deadlines.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T21:09:36.366Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:104-111
Timestamp: 2025-09-01T21:09:36.366Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 confirmed that the downloadedDocument variable and associated string-based file type detection approach should be maintained as-is because it has been working reliably in production with CW v1 for many months, and the code is primarily used for XML document parsing where binary corruption is not a concern.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-28T19:23:20.179Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-17T20:03:18.931Z
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/sequelize/migrations/2025-06-17_00_create-cohort-tables.ts:50-56
Timestamp: 2025-06-17T20:03:18.931Z
Learning: In the Metriport codebase, the patient table ID is defined as DataTypes.STRING in the database schema, not UUID. Foreign key references to patient.id should use DataTypes.STRING to maintain consistency.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T18:29:06.278Z
Learnt from: thomasyopes
PR: metriport/metriport#4458
File: packages/api/src/external/ehr/elation/command/sync-patient.ts:0-0
Timestamp: 2025-08-25T18:29:06.278Z
Learning: The `inputMetriportPatientId` parameter in `syncElationPatientIntoMetriport` and `syncHealthiePatientIntoMetriport` functions is meant for optional validation in `getOrCreateMetriportPatient`, not as a required parameter that call sites need to explicitly map. It can remain undefined/unmapped at call sites and the function will work correctly without it.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T23:29:58.588Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:276-287
Timestamp: 2025-09-01T23:29:58.588Z
Learning: RamilGaripov confirmed that the capture.error(msg, { extra: { error } }) pattern used in the current code at lines 276-287 of packages/api/src/external/commonwell-v2/document/document-query.ts is the correct and established pattern for the Metriport codebase, and suggestions to change this pattern should not be made.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-27T21:24:19.963Z
Learnt from: leite08
PR: metriport/metriport#4350
File: packages/utils/src/analytics-platform/1-fhir-to-csv.ts:129-133
Timestamp: 2025-08-27T21:24:19.963Z
Learning: In packages/utils/src/analytics-platform/1-fhir-to-csv.ts, the SQS FIFO messageDeduplicationId is intentionally set to just patientId (not scoped to jobId) to prevent any duplicate patient processing across job runs and retries, as confirmed by leite08. This ensures strict patient-level idempotency in the analytics pipeline.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-26T17:41:19.009Z
Learnt from: thomasyopes
PR: metriport/metriport#4451
File: packages/core/src/external/ehr/shared.ts:0-0
Timestamp: 2025-08-26T17:41:19.009Z
Learning: When debugging Canvas-specific issues in packages/core/src/external/ehr/shared.ts, the team now logs only the specific response headers they care about (selective logging) rather than full response headers, after initially logging everything to determine what data was valuable.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:52:21.619Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:52:11.649Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:49-58
Timestamp: 2025-08-25T17:52:11.649Z
Learning: RamilGaripov prefers to maintain consistency between v1 and v2 CommonWell implementations rather than addressing individual technical issues in isolation, even when the existing pattern has potential issues like binary data handling via string concatenation.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-17T21:24:37.077Z
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:52:14.251Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v1/document/document-contribution.ts:4-4
Timestamp: 2025-08-25T17:52:14.251Z
Learning: For the doc-contribution endpoint in packages/core/src/external/commonwell-v1/document/document-contribution.ts, RamilGaripov prefers to keep the path as `/doc-contribution/commonwell/` rather than using a versioned path like `/doc-contribution/commonwell-v1/`.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:26:51.717Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/api/src/external/commonwell-v2/command/organization/organization.ts:175-187
Timestamp: 2025-08-25T17:26:51.717Z
Learning: In the Metriport codebase, the established error handling pattern is to use capture.error(msg, { extra: { error } }) where the message is passed as the first parameter and the actual error object is included in the extra metadata. This pattern is intentionally used throughout the codebase and should not be changed to capture.error(error, { extra: {...} }).
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-18T17:26:58.012Z
Learnt from: CR
PR: metriport/metriport#0
File: .cursorrules:0-0
Timestamp: 2025-07-18T17:26:58.012Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : 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.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T19:51:11.022Z
Learnt from: CR
PR: metriport/metriport#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-01T19:51:11.022Z
Learning: Applies to **/*.{ts,tsx} : Error handling: pass the original error as cause when wrapping errors
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-21T13:52:25.436Z
Learnt from: lucasdellabella
PR: metriport/metriport#4073
File: packages/core/src/external/hl7-notification/datetime.ts:155-161
Timestamp: 2025-06-21T13:52:25.436Z
Learning: The `capture.error` function in the Metriport codebase only accepts two parameters: `error` (the error object) and an optional `captureContext` with an `extra` property for additional context. There is no `cause` property available in the capture context.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-28T02:51:35.779Z
Learnt from: leite08
PR: metriport/metriport#3857
File: packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts:67-82
Timestamp: 2025-05-28T02:51:35.779Z
Learning: In the search-lexical.ts file, the user prefers to bubble up JSON parsing errors rather than catching and logging them. When processing FHIR resources from OpenSearch results, errors should be thrown and allowed to propagate up the call stack instead of being caught and silently ignored.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-31T21:29:39.196Z
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.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-01T02:28:19.913Z
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.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-16T02:10:01.792Z
Learnt from: lucasdellabella
PR: metriport/metriport#3855
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts:88-104
Timestamp: 2025-05-16T02:10:01.792Z
Learning: When logging file paths or other strings that might contain patient identifiers like patientId or cxId, these should be masked or redacted (e.g., using regex replacement) to avoid logging Protected Health Information (PHI) to CloudWatch or error tracking systems like Sentry, ensuring HIPAA compliance.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-26T02:33:26.777Z
Learnt from: leite08
PR: metriport/metriport#4469
File: packages/fhir-converter/src/lib/workers/worker.js:75-87
Timestamp: 2025-08-26T02:33:26.777Z
Learning: User leite08 confirmed that the debug logging approach in packages/fhir-converter/src/lib/workers/worker.js was already reviewed and approved in the feature PR, even though it logs both error messages and stack traces.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-02T14:15:03.389Z
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:445-451
Timestamp: 2025-09-02T14:15:03.389Z
Learning: User leite08 prefers to address PHI-related security improvements (like removing sensitive data from error logging) in follow-up PRs rather than in the current PR, to keep changes focused and manageable.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-16T00:54:56.156Z
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-27T00:40:32.149Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-27T23:12:23.978Z
Learnt from: thomasyopes
PR: metriport/metriport#4478
File: packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts:44-47
Timestamp: 2025-08-27T23:12:23.978Z
Learning: In packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts, the intentional swallowing of HTTP 400 errors in startContributeBundlesJob via early return is by design, as confirmed by thomasyopes. This pattern should not be flagged in future reviews.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T19:51:11.022Z
Learnt from: CR
PR: metriport/metriport#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-01T19:51:11.022Z
Learning: Applies to **/*.{ts,tsx} : Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-08T19:41:36.533Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-03-19T13:41:19.957Z
Learnt from: leite08
PR: metriport/metriport#3463
File: packages/api/src/command/medical/patient/settings/create-patient-settings.ts:202-204
Timestamp: 2025-03-19T13:41:19.957Z
Learning: In the Metriport codebase, when using `capture.error` or `capture.message` functions, the order of properties in the `extra` parameter is important because Sentry may trim data beyond its payload limit. The most critical information (usually `cxId`) should be placed first, while less important data (like `facilityId`) should be placed last.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T15:52:07.834Z
Learnt from: leite08
PR: metriport/metriport#4451
File: packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts:44-45
Timestamp: 2025-08-25T15:52:07.834Z
Learning: User leite08 emphasizes the importance of logging 4xx errors for operational visibility and filtering in log insights, even when treating them as no-ops.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-04-28T23:10:42.561Z
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/command/medical/patient/patient-import/get.ts:70-74
Timestamp: 2025-04-28T23:10:42.561Z
Learning: Error messages should have static messages. Dynamic data like IDs should be added to MetriportError's `additionalInfo` property rather than interpolated into the error message string.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-18T04:51:44.941Z
Learnt from: leite08
PR: metriport/metriport#4044
File: packages/api/src/command/medical/tcm-encounter/update-tcm-encounter.ts:23-23
Timestamp: 2025-06-18T04:51:44.941Z
Learning: Error messages should be fixed, stable, and non-dynamic. Dynamic data like IDs should be moved to the `additionalInfo` property of MetriportError instead of being interpolated into the error message string.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-20T01:18:28.093Z
Learnt from: leite08
PR: metriport/metriport#4215
File: packages/core/src/command/analytics-platform/config.ts:4-13
Timestamp: 2025-07-20T01:18:28.093Z
Learning: User leite08 prefers that code improvement suggestions (like consolidating duplicated schemas) be made on feature PRs rather than release PRs. Such improvements should be deferred to follow-up PRs during the release cycle.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T21:08:47.044Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:55-61
Timestamp: 2025-09-01T21:08:47.044Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, the downloadedDocument variable uses string concatenation for chunk accumulation, which is acceptable because it's only used for XML document parsing (text-based content) when isMimeTypeXML(downloadResult.contentType) returns true. For non-XML documents, the variable is not used, so binary corruption concerns don't apply in this context.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-21T01:59:05.287Z
Learnt from: keshavsaharia
PR: metriport/metriport#4394
File: packages/lambdas/src/quest/download-response.ts:11-18
Timestamp: 2025-08-21T01:59:05.287Z
Learning: In packages/lambdas/src/quest/download-response.ts, user keshavsaharia confirmed that using DownloadResponseHandlerDirect with the default QuestFhirConverterCommandDirect (no-op/stub) is intentional. The source document conversion should happen synchronously in the same Lambda execution as it's a fast operation, rather than using the cloud converter which would process asynchronously via SQS.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
🧬 Code graph analysis (1)
packages/api/src/external/commonwell-v2/document/document-query.ts (5)
packages/core/src/external/commonwell/document/document-downloader.ts (1)
DownloadResult(20-26)packages/api/src/external/commonwell-v2/document/shared.ts (5)
getContentTypeOrUnknown(43-45)DocumentWithMetriportId(11-14)DocumentWithLocation(16-18)CWDocumentWithMetriportData(20-27)getDocPrintableDetails(47-55)packages/api/src/command/medical/document/document-query-storage-info.ts (2)
S3Info(8-15)getUrl(39-44)packages/api/src/external/commonwell-v2/document/document-query-storage-info.ts (2)
getDocToFileFunction(13-25)getS3Info(27-84)packages/api/src/external/fhir-converter/converter.ts (1)
convertCDAToFHIR(38-91)
⏰ 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). (7)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/api/src/external/commonwell-v2/document/document-query.ts (2)
256-267: Good: failures set both download and convert to "failed".Covers both tracks on overarching errors; aligns with prior decision to keep this for v2 only.
343-351: Good: avoid spread-override; final id uses masterIdentifier.value.Order prevents
d.idfrom clobbering the intendedid.
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
76-76: Good fix: gate XML parsing on the final content type.Using
newlyDownloadedFile.contentTyperesolves the earlier correctness issue after MIME updates.
🧹 Nitpick comments (1)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
52-57: Type the stream chunk; avoidany.Use a precise union and normalize to string. Keeps the proven string-accumulation approach while satisfying linting and improving safety.
-//eslint-disable-next-line @typescript-eslint/no-explicit-any -function onData(chunk: any) { - downloadedDocument += chunk; -} -function onEnd() { - log("Finished downloading document"); -} +function onData(chunk: Buffer | string): void { + downloadedDocument += typeof chunk === "string" ? chunk : chunk.toString("utf8"); +} +function onEnd(): void { + log("Finished downloading document"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
🧠 Learnings (6)
📓 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.
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-01T21:09:36.366Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:104-111
Timestamp: 2025-09-01T21:09:36.366Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 confirmed that the downloadedDocument variable and associated string-based file type detection approach should be maintained as-is because it has been working reliably in production with CW v1 for many months, and the code is primarily used for XML document parsing where binary corruption is not a concern.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-01T20:39:53.630Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:0-0
Timestamp: 2025-09-01T20:39:53.630Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain the existing string-based content accumulation approach for document downloading rather than switching to Buffer-based handling, prioritizing production stability and proven logic over technical improvements when under shipping deadlines.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-01T21:08:47.044Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:55-61
Timestamp: 2025-09-01T21:08:47.044Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, the downloadedDocument variable uses string concatenation for chunk accumulation, which is acceptable because it's only used for XML document parsing (text-based content) when isMimeTypeXML(downloadResult.contentType) returns true. For non-XML documents, the variable is not used, so binary corruption concerns don't apply in this context.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
📚 Learning: 2025-09-02T14:30:47.451Z
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Applied to files:
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
🧬 Code graph analysis (1)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (2)
packages/core/src/util/log.ts (1)
log(10-23)packages/core/src/util/mime.ts (1)
isMimeTypeXML(11-13)
🔇 Additional comments (1)
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
62-63: LGTM – explicit key names.Explicitly passing
{ document, fileInfo }is clear and consistent.
packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/api/src/external/commonwell-v2/document/document-query.ts (5)
138-150: Avoid leaving progress stuck in "processing" on early return (move setDocQueryProgress).setDocQueryProgress is called before computing isTriggerDQ; if isTriggerDQ is false, the function returns and progress remains "processing". Move the progress init to just before the download path (after the isTriggerDQ check).
- const [initiator] = await Promise.all([ - getCwInitiator(patientParam, facilityId), - setDocQueryProgress({ - patient: { id: patientId, cxId }, - downloadProgress: { status: "processing" }, - convertProgress: { status: "processing" }, - requestId, - source: MedicalDataSource.COMMONWELL, - triggerConsolidated, - }), - ]); + const initiator = await getCwInitiator(patientParam, facilityId); @@ - if (!isTriggerDQ) return; + if (!isTriggerDQ) return; + + await setDocQueryProgress({ + patient: { id: patientId, cxId }, + downloadProgress: { status: "processing" }, + convertProgress: { status: "processing" }, + requestId, + source: MedicalDataSource.COMMONWELL, + triggerConsolidated, + });Also applies to: 219-222
374-379: Sanitize OperationOutcome logging (avoid JSON dump).Log only category and count; keep full details in capture.extra.
- const msg = `CW Document query error`; - log(`${msg} - Category: ${category}. Cause: ${JSON.stringify(errors)}`); + const msg = `CW Document query error`; + log(`${msg} - Category: ${category}. count=${errors.length}`); capture.error(msg, { extra: { ...context, errors, category }, });
445-451: Don’t attach full CW doc to MetriportError (PHI).Replace JSON.stringify(document) with a minimal, non-PHI shape.
- if (!documentId) - throw new MetriportError("Document ID is required", undefined, { - extra: JSON.stringify(document), - }); + if (!documentId) { + const printable = { + id: document.masterIdentifier?.value ?? document.id, + hasUrl: Boolean(document.content?.[0]?.attachment?.url), + contentType: document.content?.[0]?.attachment?.contentType, + }; + throw new MetriportError("Document ID is required", undefined, printable); + }
633-643: Sanitize capture payload on download errors.Avoid passing the full DocumentReference; use getDocPrintableDetails(doc).
capture.error(msg, { extra: { context: `cw.downloadDocsAndUpsertFHIR.downloadFromCWAndUploadToS3`, patientId: patient.id, - documentReference: doc, + document: getDocPrintableDetails(doc), requestId, error: errorToString(error), }, });
694-703: Sanitize FHIR upsert error context.Replace documentReference: doc with a minimal printable doc summary.
- upsertDocumentToFHIRServer(cxId, fhirDocRef, log).catch(error => { + upsertDocumentToFHIRServer(cxId, fhirDocRef, log).catch(error => { const context = "upsertDocumentToFHIRServer"; const extra = { context: `cw.document-query.` + context, patientId: patient.id, - documentReference: doc, + document: getDocPrintableDetails(doc), originalError: error, };
🧹 Nitpick comments (1)
packages/api/src/external/commonwell-v2/document/document-query.ts (1)
403-416: Use standard logger for consistency and request correlation.Prefer out().log over console.log here.
- console.log( - `Doc without location, skipping - fhirDocId ${doc.id}, originalDocId ${doc.originalId}` - ); + out().log( + `Doc without location, skipping - fhirDocId ${doc.id}, originalDocId ${doc.originalId}` + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/api/src/external/commonwell-v2/document/document-query.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/api/src/external/commonwell-v2/document/document-query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/api/src/external/commonwell-v2/document/document-query.ts
🧠 Learnings (43)
📓 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.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-09-01T22:26:44.481Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts:0-0
Timestamp: 2025-09-01T22:26:44.481Z
Learning: The file packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts contains existing logic that was moved from elsewhere, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T21:54:32.973Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T19:57:53.618Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-02T14:30:47.451Z
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-20T21:26:26.804Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-21T01:50:04.471Z
Learnt from: keshavsaharia
PR: metriport/metriport#4394
File: packages/core/src/external/quest/command/fhir-converter/fhir-converter-direct.ts:5-8
Timestamp: 2025-08-21T01:50:04.471Z
Learning: In packages/core/src/external/quest/command/fhir-converter/fhir-converter-direct.ts, the QuestFhirConverterCommandDirect class is intentionally implemented as a stub/TODO for ENG-864, as confirmed by keshavsaharia. The Quest FHIR conversion feature is being developed in phases - the current PR establishes the pipeline structure, and the next PR will implement the actual conversion logic.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T20:39:53.630Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:0-0
Timestamp: 2025-09-01T20:39:53.630Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain the existing string-based content accumulation approach for document downloading rather than switching to Buffer-based handling, prioritizing production stability and proven logic over technical improvements when under shipping deadlines.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T21:09:36.366Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:104-111
Timestamp: 2025-09-01T21:09:36.366Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 confirmed that the downloadedDocument variable and associated string-based file type detection approach should be maintained as-is because it has been working reliably in production with CW v1 for many months, and the code is primarily used for XML document parsing where binary corruption is not a concern.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-28T19:23:20.179Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-17T20:03:18.931Z
Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/sequelize/migrations/2025-06-17_00_create-cohort-tables.ts:50-56
Timestamp: 2025-06-17T20:03:18.931Z
Learning: In the Metriport codebase, the patient table ID is defined as DataTypes.STRING in the database schema, not UUID. Foreign key references to patient.id should use DataTypes.STRING to maintain consistency.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T18:29:06.278Z
Learnt from: thomasyopes
PR: metriport/metriport#4458
File: packages/api/src/external/ehr/elation/command/sync-patient.ts:0-0
Timestamp: 2025-08-25T18:29:06.278Z
Learning: The `inputMetriportPatientId` parameter in `syncElationPatientIntoMetriport` and `syncHealthiePatientIntoMetriport` functions is meant for optional validation in `getOrCreateMetriportPatient`, not as a required parameter that call sites need to explicitly map. It can remain undefined/unmapped at call sites and the function will work correctly without it.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T23:29:58.588Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:276-287
Timestamp: 2025-09-01T23:29:58.588Z
Learning: RamilGaripov confirmed that the capture.error(msg, { extra: { error } }) pattern used in the current code at lines 276-287 of packages/api/src/external/commonwell-v2/document/document-query.ts is the correct and established pattern for the Metriport codebase, and suggestions to change this pattern should not be made.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-27T21:24:19.963Z
Learnt from: leite08
PR: metriport/metriport#4350
File: packages/utils/src/analytics-platform/1-fhir-to-csv.ts:129-133
Timestamp: 2025-08-27T21:24:19.963Z
Learning: In packages/utils/src/analytics-platform/1-fhir-to-csv.ts, the SQS FIFO messageDeduplicationId is intentionally set to just patientId (not scoped to jobId) to prevent any duplicate patient processing across job runs and retries, as confirmed by leite08. This ensures strict patient-level idempotency in the analytics pipeline.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-26T17:41:19.009Z
Learnt from: thomasyopes
PR: metriport/metriport#4451
File: packages/core/src/external/ehr/shared.ts:0-0
Timestamp: 2025-08-26T17:41:19.009Z
Learning: When debugging Canvas-specific issues in packages/core/src/external/ehr/shared.ts, the team now logs only the specific response headers they care about (selective logging) rather than full response headers, after initially logging everything to determine what data was valuable.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:52:21.619Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:52:11.649Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:49-58
Timestamp: 2025-08-25T17:52:11.649Z
Learning: RamilGaripov prefers to maintain consistency between v1 and v2 CommonWell implementations rather than addressing individual technical issues in isolation, even when the existing pattern has potential issues like binary data handling via string concatenation.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-17T21:24:37.077Z
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:52:14.251Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v1/document/document-contribution.ts:4-4
Timestamp: 2025-08-25T17:52:14.251Z
Learning: For the doc-contribution endpoint in packages/core/src/external/commonwell-v1/document/document-contribution.ts, RamilGaripov prefers to keep the path as `/doc-contribution/commonwell/` rather than using a versioned path like `/doc-contribution/commonwell-v1/`.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T20:04:05.083Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts:81-91
Timestamp: 2025-09-01T20:04:05.083Z
Learning: In CommonWell v2 document conversion (packages/api/src/external/commonwell-v2/document/cw-to-fhir.ts), the doc.status field can be used directly without additional validation because the underlying CommonWell SDK v2 already provides schema validation that ensures DocumentReference.status values conform to FHIR R4 requirements ("current" | "superseded" | "entered-in-error").
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-26T02:33:26.777Z
Learnt from: leite08
PR: metriport/metriport#4469
File: packages/fhir-converter/src/lib/workers/worker.js:75-87
Timestamp: 2025-08-26T02:33:26.777Z
Learning: User leite08 confirmed that the debug logging approach in packages/fhir-converter/src/lib/workers/worker.js was already reviewed and approved in the feature PR, even though it logs both error messages and stack traces.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T17:26:51.717Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/api/src/external/commonwell-v2/command/organization/organization.ts:175-187
Timestamp: 2025-08-25T17:26:51.717Z
Learning: In the Metriport codebase, the established error handling pattern is to use capture.error(msg, { extra: { error } }) where the message is passed as the first parameter and the actual error object is included in the extra metadata. This pattern is intentionally used throughout the codebase and should not be changed to capture.error(error, { extra: {...} }).
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-18T17:26:58.012Z
Learnt from: CR
PR: metriport/metriport#0
File: .cursorrules:0-0
Timestamp: 2025-07-18T17:26:58.012Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : 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.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T19:51:11.022Z
Learnt from: CR
PR: metriport/metriport#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-01T19:51:11.022Z
Learning: Applies to **/*.{ts,tsx} : Error handling: pass the original error as cause when wrapping errors
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-21T13:52:25.436Z
Learnt from: lucasdellabella
PR: metriport/metriport#4073
File: packages/core/src/external/hl7-notification/datetime.ts:155-161
Timestamp: 2025-06-21T13:52:25.436Z
Learning: The `capture.error` function in the Metriport codebase only accepts two parameters: `error` (the error object) and an optional `captureContext` with an `extra` property for additional context. There is no `cause` property available in the capture context.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-28T02:51:35.779Z
Learnt from: leite08
PR: metriport/metriport#3857
File: packages/core/src/command/consolidated/search/fhir-resource/search-lexical.ts:67-82
Timestamp: 2025-05-28T02:51:35.779Z
Learning: In the search-lexical.ts file, the user prefers to bubble up JSON parsing errors rather than catching and logging them. When processing FHIR resources from OpenSearch results, errors should be thrown and allowed to propagate up the call stack instead of being caught and silently ignored.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-31T21:29:39.196Z
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.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-01T02:28:19.913Z
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.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-16T02:10:01.792Z
Learnt from: lucasdellabella
PR: metriport/metriport#3855
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts:88-104
Timestamp: 2025-05-16T02:10:01.792Z
Learning: When logging file paths or other strings that might contain patient identifiers like patientId or cxId, these should be masked or redacted (e.g., using regex replacement) to avoid logging Protected Health Information (PHI) to CloudWatch or error tracking systems like Sentry, ensuring HIPAA compliance.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-02T14:15:03.389Z
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:445-451
Timestamp: 2025-09-02T14:15:03.389Z
Learning: User leite08 prefers to address PHI-related security improvements (like removing sensitive data from error logging) in follow-up PRs rather than in the current PR, to keep changes focused and manageable.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-16T00:54:56.156Z
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-27T00:40:32.149Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-27T23:12:23.978Z
Learnt from: thomasyopes
PR: metriport/metriport#4478
File: packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts:44-47
Timestamp: 2025-08-27T23:12:23.978Z
Learning: In packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts, the intentional swallowing of HTTP 400 errors in startContributeBundlesJob via early return is by design, as confirmed by thomasyopes. This pattern should not be flagged in future reviews.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T19:51:11.022Z
Learnt from: CR
PR: metriport/metriport#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-01T19:51:11.022Z
Learning: Applies to **/*.{ts,tsx} : Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-05-08T19:41:36.533Z
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/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-03-19T13:41:19.957Z
Learnt from: leite08
PR: metriport/metriport#3463
File: packages/api/src/command/medical/patient/settings/create-patient-settings.ts:202-204
Timestamp: 2025-03-19T13:41:19.957Z
Learning: In the Metriport codebase, when using `capture.error` or `capture.message` functions, the order of properties in the `extra` parameter is important because Sentry may trim data beyond its payload limit. The most critical information (usually `cxId`) should be placed first, while less important data (like `facilityId`) should be placed last.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-25T15:52:07.834Z
Learnt from: leite08
PR: metriport/metriport#4451
File: packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts:44-45
Timestamp: 2025-08-25T15:52:07.834Z
Learning: User leite08 emphasizes the importance of logging 4xx errors for operational visibility and filtering in log insights, even when treating them as no-ops.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-04-28T23:10:42.561Z
Learnt from: leite08
PR: metriport/metriport#3749
File: packages/api/src/command/medical/patient/patient-import/get.ts:70-74
Timestamp: 2025-04-28T23:10:42.561Z
Learning: Error messages should have static messages. Dynamic data like IDs should be added to MetriportError's `additionalInfo` property rather than interpolated into the error message string.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-06-18T04:51:44.941Z
Learnt from: leite08
PR: metriport/metriport#4044
File: packages/api/src/command/medical/tcm-encounter/update-tcm-encounter.ts:23-23
Timestamp: 2025-06-18T04:51:44.941Z
Learning: Error messages should be fixed, stable, and non-dynamic. Dynamic data like IDs should be moved to the `additionalInfo` property of MetriportError instead of being interpolated into the error message string.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-07-20T01:18:28.093Z
Learnt from: leite08
PR: metriport/metriport#4215
File: packages/core/src/command/analytics-platform/config.ts:4-13
Timestamp: 2025-07-20T01:18:28.093Z
Learning: User leite08 prefers that code improvement suggestions (like consolidating duplicated schemas) be made on feature PRs rather than release PRs. Such improvements should be deferred to follow-up PRs during the release cycle.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-09-01T21:08:47.044Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:55-61
Timestamp: 2025-09-01T21:08:47.044Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, the downloadedDocument variable uses string concatenation for chunk accumulation, which is acceptable because it's only used for XML document parsing (text-based content) when isMimeTypeXML(downloadResult.contentType) returns true. For non-XML documents, the variable is not used, so binary corruption concerns don't apply in this context.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
📚 Learning: 2025-08-21T01:59:05.287Z
Learnt from: keshavsaharia
PR: metriport/metriport#4394
File: packages/lambdas/src/quest/download-response.ts:11-18
Timestamp: 2025-08-21T01:59:05.287Z
Learning: In packages/lambdas/src/quest/download-response.ts, user keshavsaharia confirmed that using DownloadResponseHandlerDirect with the default QuestFhirConverterCommandDirect (no-op/stub) is intentional. The source document conversion should happen synchronously in the same Lambda execution as it's a fast operation, rather than using the cloud converter which would process asynchronously via SQS.
Applied to files:
packages/api/src/external/commonwell-v2/document/document-query.ts
🧬 Code graph analysis (1)
packages/api/src/external/commonwell-v2/document/document-query.ts (11)
packages/core/src/external/commonwell/document/document-downloader.ts (2)
DownloadResult(20-26)Document(9-13)packages/core/src/util/log.ts (3)
out(30-35)log(10-23)debug(25-28)packages/api/src/external/commonwell/shared.ts (1)
validateCWEnabled(57-117)packages/core/src/command/feature-flags/domain-ffs.ts (3)
isStalePatientUpdateEnabledForCx(191-194)isEnhancedCoverageEnabledForCx(139-142)isCQDirectEnabledForCx(147-150)packages/core/src/util/error/shared.ts (1)
processAsyncError(57-71)packages/api/src/external/commonwell/patient/patient-external-data.ts (2)
getPatientWithCWData(48-56)PatientWithCWData(19-21)packages/api/src/external/commonwell-v2/document/shared.ts (5)
getContentTypeOrUnknown(43-45)DocumentWithMetriportId(11-14)DocumentWithLocation(16-18)CWDocumentWithMetriportData(20-27)getDocPrintableDetails(47-55)packages/api/src/command/medical/document/document-query-storage-info.ts (2)
S3Info(8-15)getUrl(39-44)packages/api/src/external/commonwell-v2/document/document-query-storage-info.ts (2)
getDocToFileFunction(13-25)getS3Info(27-84)packages/api/src/external/fhir-converter/converter.ts (1)
convertCDAToFHIR(38-91)packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts (1)
cwToFHIR(49-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: check-pr / lint-build-test
🔇 Additional comments (2)
packages/api/src/external/commonwell-v2/document/document-query.ts (2)
343-353: Spread-order bug fixed correctly.Spreading first and then overriding id with masterIdentifier.value keeps the intended key.
744-756: Good PHI-safe logging.Using getDocPrintableDetails and single-line log keeps logs safe and readable.
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/commonwell-sdk/src/client/commonwell.ts (2)
162-166: Unify getPatient overload to never return undefined.Implementation and schema guarantee a nonempty Patients array; align the overload.
Apply:
- ): Promise<PatientResponseItem | undefined>; + ): Promise<PatientResponseItem>;Then check call sites that expect undefined:
#!/bin/bash # Find getPatient calls and look for undefined checks rg -nP -C2 '\bgetPatient\s*\(' --type=ts rg -nP -C2 '\bgetPatient\s*\([^)]*\)\s*(?:\.then|\)|;)' --type=ts rg -nP -C2 '\bif\s*\(\s*await\s+.*getPatient\([^)]*\)\s*==?\s*undefined' --type=ts
600-616: Return a concrete Error from getDescriptiveError.Prevents throwing non-Error values and improves type-safety.
- private getDescriptiveError(error: unknown, title: string): unknown { + private getDescriptiveError(error: unknown, title: string): Error { if (isAxiosError(error)) { const status = error.response?.status; const data = error.response?.data; const responseBody = data ? JSON.stringify(data) : undefined; const cwReference = this.lastTransactionId; if (status === httpStatus.BAD_REQUEST) { return new BadRequestError(title, error, { status, cwReference, responseBody }); } if (status === httpStatus.NOT_FOUND) { return new NotFoundError(title, error, { status, cwReference, responseBody }); } return new MetriportError(title, error, { status, cwReference, responseBody }); } - return error; + return error instanceof Error ? error : new MetriportError(title, error); }If this was “fixed upstream,” please rebase/merge to bring the change into this branch and confirm no callers rely on a non-Error return.
🧹 Nitpick comments (3)
packages/commonwell-sdk/src/client/commonwell.ts (3)
196-201: Prefer MetriportError for invalid-argument throw.Keeps error shape consistent across the SDK.
- if (!idOrParams) { - throw new Error("Programming error: 'id' is required when providing separate parameters"); - } + if (!idOrParams) { + throw new MetriportError( + "Invalid argument: 'id' is required when providing separate parameters" + ); + }
280-283: Fix JSDoc grammar for patientId.- * @param patientId The person id to be link to a patient. + * @param patientId The local patient ID whose links to other patients are requested.
452-457: Avoid explicit any: return unknown and drop eslint-disable.Callers already parse with zod; unknown is safer and meets TS guidelines.
- private async queryDocumentsInternal( - patientId: string, - options?: BaseOptions & DocumentQueryParams - //eslint-disable-next-line @typescript-eslint/no-explicit-any - ): Promise<any> { + private async queryDocumentsInternal( + patientId: string, + options?: BaseOptions & DocumentQueryParams + ): Promise<unknown> {Also applies to: 463-466
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
packages/api/src/external/commonwell-v1/document/__tests__/index-to-fhir.test.ts(1 hunks)packages/api/src/external/commonwell-v1/document/document-query.ts(3 hunks)packages/commonwell-cert-runner/src/single-commands/patient-delete.ts(2 hunks)packages/commonwell-cert-runner/src/single-commands/patient-get-links.ts(2 hunks)packages/commonwell-cert-runner/src/single-commands/patient-get.ts(2 hunks)packages/commonwell-sdk/src/client/commonwell.ts(9 hunks)packages/utils/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/commonwell-cert-runner/src/single-commands/patient-get.ts
- packages/api/src/external/commonwell-v1/document/tests/index-to-fhir.test.ts
- packages/commonwell-cert-runner/src/single-commands/patient-get-links.ts
- packages/utils/package.json
- packages/commonwell-cert-runner/src/single-commands/patient-delete.ts
- packages/api/src/external/commonwell-v1/document/document-query.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/commonwell-sdk/src/client/commonwell.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/commonwell-sdk/src/client/commonwell.ts
🧠 Learnings (30)
📓 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.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-05-20T21:26:26.804Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T19:57:53.618Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-06-27T01:50:14.227Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-06-06T16:45:31.832Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:16:42.635Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.635Z
Learning: In packages/commonwell-sdk/src/models/demographics.ts, the demographicsSchema defines the name field as z.array(humanNameSchema) without .optional() or .nullish(), which means patient.name is always present as an array and will never be undefined. This applies to all Patient types in CommonWell SDK including those used in PatientExistingLink and PatientProbableLink. Defensive programming with optional chaining is not needed when accessing patient.name in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:16:42.635Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.635Z
Learning: In packages/commonwell-sdk/src/models/patient.ts, the Patient schema defines the name field as z.array(humanNameSchema) without .optional() or .nullish(), which means patient.name is always present as an array and will never be undefined. Defensive programming with optional chaining is not needed when accessing patient.name in CommonWell v2 patient data structures like PatientExistingLink and PatientProbableLink.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T19:51:11.022Z
Learnt from: CR
PR: metriport/metriport#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-09-01T19:51:11.022Z
Learning: Applies to **/*.{ts,tsx} : Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-26T02:33:26.777Z
Learnt from: leite08
PR: metriport/metriport#4469
File: packages/fhir-converter/src/lib/workers/worker.js:75-87
Timestamp: 2025-08-26T02:33:26.777Z
Learning: User leite08 confirmed that the debug logging approach in packages/fhir-converter/src/lib/workers/worker.js was already reviewed and approved in the feature PR, even though it logs both error messages and stack traces.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T23:29:58.588Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:276-287
Timestamp: 2025-09-01T23:29:58.588Z
Learning: RamilGaripov confirmed that the capture.error(msg, { extra: { error } }) pattern used in the current code at lines 276-287 of packages/api/src/external/commonwell-v2/document/document-query.ts is the correct and established pattern for the Metriport codebase, and suggestions to change this pattern should not be made.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-25T17:52:19.940Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/commonwell-sdk/src/client/commonwell-member.ts:231-233
Timestamp: 2025-08-25T17:52:19.940Z
Learning: In packages/commonwell-sdk/src/client/commonwell-member.ts, the getOneOrg method currently returns undefined for all non-404 HTTP errors (instead of throwing MetriportError) as a temporary workaround while waiting for the service provider to fix issues on their end. This is tracked in TODO ENG-668 and will be reverted later once the external dependency is resolved.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-16T00:54:56.156Z
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-27T23:12:23.978Z
Learnt from: thomasyopes
PR: metriport/metriport#4478
File: packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts:44-47
Timestamp: 2025-08-27T23:12:23.978Z
Learning: In packages/core/src/external/ehr/api/job/start-contribute-bundles-job.ts, the intentional swallowing of HTTP 400 errors in startContributeBundlesJob via early return is by design, as confirmed by thomasyopes. This pattern should not be flagged in future reviews.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:26:44.481Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts:0-0
Timestamp: 2025-09-01T22:26:44.481Z
Learning: The file packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts contains existing logic that was moved from elsewhere, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T21:54:32.973Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-06-03T21:02:23.374Z
Learnt from: leite08
PR: metriport/metriport#3953
File: packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts:28-30
Timestamp: 2025-06-03T21:02:23.374Z
Learning: The `makePatient()` function returns `PatientWithId` type which requires the `id` field to be present, ensuring `patient.id` is never undefined.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-14T23:29:53.606Z
Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/core/src/external/quest/file/file-generator.ts:55-57
Timestamp: 2025-08-14T23:29:53.606Z
Learning: In packages/shared/src/domain/patient.ts, the Patient schema defines address as z.array(...) without .optional(), meaning the address field is always present as an array. Optional chaining is not needed when accessing patient.address[0], though checking for undefined after indexing is still necessary in case the array is empty.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T23:31:30.405Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient.ts:850-886
Timestamp: 2025-09-01T23:31:30.405Z
Learning: In packages/commonwell-sdk/src/models/patient.ts, the Patient schema defines name as z.array(humanNameSchema), address as z.array(addressSchema), and identifier as z.array(identifierSchema) without .optional() or .nullish(), which means these fields are always present as arrays and will never be undefined. However, telecom is defined with .nullish() so it can be undefined and requires optional chaining when accessing it in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-16T18:51:08.416Z
Learnt from: leite08
PR: metriport/metriport#4095
File: packages/commonwell-cert-runner/src/single-commands/patient-create.ts:35-36
Timestamp: 2025-07-16T18:51:08.416Z
Learning: In packages/commonwell-cert-runner and the CommonWell SDK, PatientCollection.Patients is strongly typed as an array (z.array(patientCollectionItemSchema)), so resp.Patients will always exist and be an array. The getMetriportPatientIdOrFail utility function is designed to handle undefined/null input with proper error messages, making additional bounds checking unnecessary.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-14T23:29:53.606Z
Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/core/src/external/quest/file/file-generator.ts:55-57
Timestamp: 2025-08-14T23:29:53.606Z
Learning: In packages/shared/src/domain/patient.ts, the Patient type's address field is required (not optional) and is defined as z.array(addressSchema), guaranteeing it's always present as an array, so optional chaining is not needed when accessing patient.address[0].
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-16T01:38:14.080Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-05-27T16:10:48.223Z
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.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-06-01T02:28:19.913Z
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.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-05-08T19:41:36.533Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-06-18T21:05:22.256Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-15T00:00:45.080Z
Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/shared/src/domain/patient.ts:5-5
Timestamp: 2025-08-15T00:00:45.080Z
Learning: The patientSchema in packages/shared/src/domain/patient.ts is used in a limited subsystem scope ("SS") and is not the same as other Patient schemas referenced throughout the codebase, making additive optional field changes like externalId safe and non-breaking.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:15:56.485Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:24-34
Timestamp: 2025-09-01T22:15:56.485Z
Learning: In packages/api/src/external/commonwell-v2/patient/patient-demographics.ts, the NetworkLink.Patient.address field is guaranteed to be defined as it's a required array field in the CommonWell v2 Patient schema, so optional chaining is not needed when accessing this property.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-07-18T09:33:29.581Z
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/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-09-01T22:06:31.369Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-conversion.ts:60-71
Timestamp: 2025-09-01T22:06:31.369Z
Learning: In packages/api/src/external/commonwell-v2/patient/patient-conversion.ts, the patient.data.address field is guaranteed to be defined as it's a required array field in the Patient schema, so optional chaining is not needed when accessing this property.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
🧬 Code graph analysis (1)
packages/commonwell-sdk/src/client/commonwell.ts (4)
packages/commonwell-sdk/src/models/patient.ts (8)
Patient(43-43)PatientCreateOrUpdateResp(72-72)patientResponseSchema(60-63)PatientResponseItem(58-58)PatientExistingLinks(92-92)patientExistingLinksSchema(88-91)PatientProbableLinks(112-112)patientProbableLinksRespSchema(108-111)packages/commonwell-sdk/src/client/commonwell-api.ts (1)
GetPatientParams(47-54)packages/commonwell-sdk/src/common/util.ts (2)
encodeCwPatientId(61-71)encodePatientIdForDocumentExchange(97-111)packages/shared/src/index.ts (3)
MetriportError(43-43)BadRequestError(42-42)NotFoundError(44-44)
⏰ 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). (7)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/commonwell-sdk/src/client/commonwell.ts (3)
2-9: Imports and model migrations look correct.New imports align with the updated schemas/types and helpers; no issues.
Also applies to: 17-19, 21-21, 30-36
134-137: createOrUpdatePatient transformation LGTM.Parsing with patientResponseSchema and returning Links/status is clear and minimal.
Also applies to: 142-150
364-366: Demographics typing and query param mapping LGTM.GenderCodes usage and URLSearchParams appends are correct.
Also applies to: 374-375
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts (2)
118-124: Avoid MIME detection from a string for unknown/binary content.detectFileType(Buffer.from(downloadedDocument)) relies on a string that may have been produced from binary chunks, risking signature corruption for non-XML files with unaccepted/unknown source types. Prefer detecting from raw bytes (e.g., read the first N bytes from S3 via a Range request) instead of the string. If you want something minimal now, at least guard against empty strings to skip false updates.
Happy to draft a small helper in S3Utils to fetch the first 512–4096 bytes and run detectFileType on that Buffer.
117-117: Nit: follow camelCase for local vars.Rename old_extension → oldExtension to match codebase conventions.
Apply:
- const old_extension = path.extname(destinationFileInfo.name); + const oldExtension = path.extname(destinationFileInfo.name); @@ - if (mimeType === sourceDocument.mimeType || old_extension === fileExtension) { + if (mimeType === sourceDocument.mimeType || oldExtension === fileExtension) {Also applies to: 121-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/api/src/external/commonwell-v1/admin/recreate-patients-at-hies.ts(3 hunks)packages/api/src/external/commonwell-v2/document/document-query.ts(1 hunks)packages/core/src/external/commonwell-v1/document/document-downloader-local.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/api/src/external/commonwell-v2/document/document-query.ts
- packages/api/src/external/commonwell-v1/admin/recreate-patients-at-hies.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/core/src/external/commonwell-v1/document/document-downloader-local.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
🧠 Learnings (7)
📓 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.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-09-01T21:09:36.366Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:104-111
Timestamp: 2025-09-01T21:09:36.366Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 confirmed that the downloadedDocument variable and associated string-based file type detection approach should be maintained as-is because it has been working reliably in production with CW v1 for many months, and the code is primarily used for XML document parsing where binary corruption is not a concern.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-01T21:08:47.044Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:55-61
Timestamp: 2025-09-01T21:08:47.044Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, the downloadedDocument variable uses string concatenation for chunk accumulation, which is acceptable because it's only used for XML document parsing (text-based content) when isMimeTypeXML(downloadResult.contentType) returns true. For non-XML documents, the variable is not used, so binary corruption concerns don't apply in this context.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-01T20:39:53.630Z
Learnt from: leite08
PR: metriport/metriport#4511
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:0-0
Timestamp: 2025-09-01T20:39:53.630Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain the existing string-based content accumulation approach for document downloading rather than switching to Buffer-based handling, prioritizing production stability and proven logic over technical improvements when under shipping deadlines.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-02T14:30:47.451Z
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
📚 Learning: 2025-09-01T22:26:44.481Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts:0-0
Timestamp: 2025-09-01T22:26:44.481Z
Learning: The file packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts contains existing logic that was moved from elsewhere, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts
🧬 Code graph analysis (1)
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts (1)
packages/core/src/util/mime.ts (1)
isMimeTypeXML(11-13)
⏰ 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). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/core/src/external/commonwell-v1/document/document-downloader-local.ts (1)
87-93: Using the finalized MIME type to gate XML parsing is correct.Switching the check to newlyDownloadedFile.contentType aligns parsing with any post-download MIME corrections from checkAndUpdateMimeType and prevents misclassification. Looks good.
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
thomasyopes
approved these changes
Sep 3, 2025
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
Ref: #000 - @metriport/commonwell-sdk@6.3.0 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/commonwell-sdk/src/models/demographics.ts (1)
43-68: Verify 'un' mapping; also unify error payload formatting
- Ambiguity: "un" often means "unknown" (some systems) and "undifferentiated" (others). Today it maps to O(ther). Please confirm against the CW feed; if ambiguous, prefer U(unknown) or drop "un" entirely to avoid misclassification.
Apply one of:
- case "o": - case "other": - case "un": - case "undifferentiated": - return GenderCodes.O; + case "o": + case "other": + case "undifferentiated": + return GenderCodes.O; + case "un": + return GenderCodes.U;or:
- case "un":
- Consistency: include String(value) in both throws so additionalInfo is uniform.
- throw new MetriportError(`Invalid gender`, undefined, { value }); + throw new MetriportError(`Invalid gender`, undefined, { value: String(value) });
- Optional: consider adding common synonyms to O (e.g., "nonbinary", "non-binary", "nb") if they appear in inbound data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/commonwell-sdk/package.json(1 hunks)packages/commonwell-sdk/src/models/demographics.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/commonwell-sdk/package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/commonwell-sdk/src/models/demographics.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
**/*.{ts,tsx}: Prefer functional programming style functions: small, deterministic, single input and output
Avoid modifying objects received as parameters
Naming: classes and enums use PascalCase
Naming: constants, variables, and functions use camelCase
File names use kebab-case
Avoid negative names (e.g., prefer isDisabled to notEnabled)
Include units in numeric variable names when the type doesn’t convey the unit
TypeScript: use explicit types
TypeScript: prefer const over let
TypeScript: avoid any and casting from any to other types
TypeScript: use type predicates only for narrowing, not for full type coercion
TypeScript: prefer destructuring function parameters instead of multiple same-typed parameters
TypeScript: don’t use null inside the app; only at external boundaries and convert to undefined internally
TypeScript: use async/await instead of .then()
TypeScript: use strict equality ===; do not use ==
When calling a Promise-returning function without awaiting, chain .catch() to handle errors
Date/Time: always use buildDayjs() to create dayjs instances
Date/Time: prefer dayjs.duration(...) and keep values as duration
Prefer nullish coalescing (??) over OR (||) for default values
Avoid creating arrow functions
Use truthy property checks instead of the in operator (e.g., if (data.link))
Error handling: pass the original error as cause when wrapping errors
Error handling: use static error messages; put dynamic data in MetriportError.additionalInfo; error object last parameter
Global constants: move literals to constants after imports (avoid magic numbers)
Avoid multi-line logs: do not pass objects as the second parameter to console.log() or out().log()
Avoid multi-line logs: do not create multi-line strings with JSON.stringify()
Max column length is 100 characters
Use multi-line comments with /** */
Files:
packages/commonwell-sdk/src/models/demographics.ts
🧠 Learnings (17)
📓 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.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-09-01T23:31:30.405Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient.ts:850-886
Timestamp: 2025-09-01T23:31:30.405Z
Learning: In packages/commonwell-sdk/src/models/demographics.ts, the Demographics schema (which Patient extends) defines identifier as z.array(patientIdentifierSchema).min(1), name as z.array(humanNameSchema).min(1), and address as z.array(addressSchema).min(1), which guarantees these fields are always present as non-empty arrays and will never be undefined. However, telecom is defined as z.array(contactSchema).nullish() so it can be undefined and requires optional chaining when accessing it in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-01T22:16:42.635Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.635Z
Learning: In packages/commonwell-sdk/src/models/demographics.ts, the demographicsSchema defines the name field as z.array(humanNameSchema) without .optional() or .nullish(), which means patient.name is always present as an array and will never be undefined. This applies to all Patient types in CommonWell SDK including those used in PatientExistingLink and PatientProbableLink. Defensive programming with optional chaining is not needed when accessing patient.name in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-07-09T17:18:16.731Z
Learnt from: thomasyopes
PR: metriport/metriport#4164
File: packages/core/src/external/ehr/healthie/index.ts:939-953
Timestamp: 2025-07-09T17:18:16.731Z
Learning: In packages/core/src/external/ehr/healthie/index.ts, the immunization.cvx_code field is required and non-nullable according to the zod schema validation (z.coerce.string()), so null checks are not needed in the convertImmunizationToFhir method.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-01T22:16:42.635Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.635Z
Learning: In packages/commonwell-sdk/src/models/patient.ts, the Patient schema defines the name field as z.array(humanNameSchema) without .optional() or .nullish(), which means patient.name is always present as an array and will never be undefined. Defensive programming with optional chaining is not needed when accessing patient.name in CommonWell v2 patient data structures like PatientExistingLink and PatientProbableLink.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-07-18T17:26:58.012Z
Learnt from: CR
PR: metriport/metriport#0
File: .cursorrules:0-0
Timestamp: 2025-07-18T17:26:58.012Z
Learning: Applies to **/*.{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
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-01T23:31:30.405Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient.ts:850-886
Timestamp: 2025-09-01T23:31:30.405Z
Learning: In packages/commonwell-sdk/src/models/patient.ts, the Patient schema defines name as z.array(humanNameSchema), address as z.array(addressSchema), and identifier as z.array(identifierSchema) without .optional() or .nullish(), which means these fields are always present as arrays and will never be undefined. However, telecom is defined with .nullish() so it can be undefined and requires optional chaining when accessing it in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-08-26T02:33:26.777Z
Learnt from: leite08
PR: metriport/metriport#4469
File: packages/fhir-converter/src/lib/workers/worker.js:75-87
Timestamp: 2025-08-26T02:33:26.777Z
Learning: User leite08 confirmed that the debug logging approach in packages/fhir-converter/src/lib/workers/worker.js was already reviewed and approved in the feature PR, even though it logs both error messages and stack traces.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-01T19:57:53.618Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts:468-479
Timestamp: 2025-09-01T19:57:53.618Z
Learning: The file packages/api/src/external/commonwell-v1/document/cw-to-fhir.ts contains existing logic copied over from packages/api/src/external/fhir/document/index.ts, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-01T22:26:44.481Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts:0-0
Timestamp: 2025-09-01T22:26:44.481Z
Learning: The file packages/api/src/external/commonwell-v2/document/document-query-sandbox.ts contains existing logic that was moved from elsewhere, so issues in this file are out of scope for code review as they represent moved code rather than new functionality.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-01T22:16:42.636Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/patient/patient-demographics.ts:17-23
Timestamp: 2025-09-01T22:16:42.636Z
Learning: In packages/commonwell-sdk/src/models/demographics.ts and human-name.ts, the schema guarantees that patient.name is z.array(humanNameSchema).min(1) (always an array with at least 1 element), and within each name object, both family and given are z.array(z.string()) (always arrays, never undefined). This means defensive programming with optional chaining or null checks is unnecessary when accessing patient.name, name.family, or name.given in CommonWell v2 patient data structures.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-07-16T00:54:56.156Z
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-05-20T21:26:26.804Z
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/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-02T14:15:01.428Z
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-01T21:54:32.973Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-02T01:44:06.297Z
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
📚 Learning: 2025-09-02T20:02:57.385Z
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:374-378
Timestamp: 2025-09-02T20:02:57.385Z
Learning: In packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov prefers to maintain the existing OperationOutcome logging pattern from v1 (including JSON.stringify of full error objects) rather than implementing PHI sanitization improvements, prioritizing consistency between v1 and v2 implementations during the CW v2 migration period.
Applied to files:
packages/commonwell-sdk/src/models/demographics.ts
🧬 Code graph analysis (1)
packages/commonwell-sdk/src/models/demographics.ts (1)
packages/shared/src/index.ts (1)
MetriportError(43-43)
⏰ 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). (17)
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-branch
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (2)
packages/commonwell-sdk/src/models/demographics.ts (2)
15-18: Gender enum switched to "F/M/O/U" — LGTMConsistent with CW v2 expectations and the PR objectives.
21-25: Preprocess now correctly accepts null/empty inputsReturning undefined here avoids the prior preprocess/.nullish() trap; schema shape is clean.
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/lambdas/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
packages/lambdas/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:275-288
Timestamp: 2025-08-25T17:52:21.619Z
Learning: RamilGaripov prefers to maintain consistency between CommonWell v1 and v2 implementations during the CW v2 API migration, keeping the new v2 files nearly identical to existing v1 files rather than making isolated improvements.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Learnt from: leite08
PR: metriport/metriport#4470
File: packages/commonwell-sdk/src/models/demographics.ts:15-18
Timestamp: 2025-09-02T14:15:01.428Z
Learning: Carequality (CQ) changes are typically handled separately from CommonWell (CW) changes and should not be mixed in the same PR. There is a dedicated Linear issue ENG-978 for CQ to support other genders, separate from CW v2 migration work.
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api/src/external/commonwell-v1/document/document-query.ts:815-821
Timestamp: 2025-09-02T14:30:47.451Z
Learning: In packages/api/src/external/commonwell-v1/document/document-query.ts, the docDownloader.download call includes cxId parameter despite being in a v1 file path because makeDocumentDownloader can route to CW v2 implementation based on feature flags, and the v2 downloader does support the cxId parameter. User leite08 confirmed this routing is working correctly in the CW v2 reintroduction PR.
Learnt from: RamilGaripov
PR: metriport/metriport#4470
File: packages/api/src/external/commonwell-v2/document/document-query.ts:139-151
Timestamp: 2025-09-01T21:54:32.973Z
Learning: During the CW v2 migration in packages/api/src/external/commonwell-v2/document/document-query.ts, RamilGaripov chose to maintain consistency with the v1 implementation rather than fixing the setDocQueryProgress placement issue, preferring to keep both versions aligned during the migration period.
Learnt from: leite08
PR: metriport/metriport#4520
File: packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts:115-126
Timestamp: 2025-09-02T01:44:06.297Z
Learning: In packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts, user leite08 prefers to maintain consistency with v1 implementation even when potential logical improvements are identified, prioritizing stability during release cycles and deferring such changes to follow-up feature PRs.
Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.
📚 Learning: 2025-03-19T13:58:17.253Z
Learnt from: leite08
PR: metriport/metriport#3463
File: packages/api-sdk/src/medical/models/patient.ts:1-1
Timestamp: 2025-03-19T13:58:17.253Z
Learning: When changes are made to SDK packages (`api-sdk`, `commonwell-sdk`, `carequality-sdk`) or `packages/shared` in the Metriport codebase, alpha versions need to be published to NPM before merging the PR.
Applied to files:
packages/lambdas/package.json
📚 Learning: 2025-09-02T14:29:51.924Z
Learnt from: leite08
PR: metriport/metriport#4526
File: packages/api-sdk/package.json:3-3
Timestamp: 2025-09-02T14:29:51.924Z
Learning: In the Metriport codebase, SDK packages (api-sdk, commonwell-sdk, carequality-sdk, etc.) use local file dependencies (e.g., "metriport/api-sdk": "file:packages/api-sdk") rather than published semver versions in dependent package.json files. This is the expected dependency management pattern.
Applied to files:
packages/lambdas/package.json
📚 Learning: 2025-08-13T16:08:03.706Z
Learnt from: RadmirGari
PR: metriport/metriport#4324
File: packages/core/src/domain/address.ts:7-7
Timestamp: 2025-08-13T16:08:03.706Z
Learning: In the Metriport codebase, packages/core legitimately depends on metriport/api-sdk and imports from it in multiple places. This is an established architectural pattern and is properly declared as a dependency in core's package.json.
Applied to files:
packages/lambdas/package.json
📚 Learning: 2025-07-15T19:00:17.445Z
Learnt from: leite08
PR: metriport/metriport#4190
File: packages/shared/package.json:3-3
Timestamp: 2025-07-15T19:00:17.445Z
Learning: The connect-widget package in the Metriport codebase is no longer being updated, so alpha version references in its package.json can be left as-is during release processes.
Applied to files:
packages/lambdas/package.json
📚 Learning: 2025-08-20T22:28:47.373Z
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:28:47.373Z
Learning: In the Metriport codebase, form-data dependencies with version ">=4.0.4" are intentionally added to package.json files (even when not directly used) to force NPM to use secure versions and address Dependabot security alerts for transitive dependencies.
Applied to files:
packages/lambdas/package.json
📚 Learning: 2025-08-20T22:19:04.553Z
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-08-20T22:19:04.553Z
Learning: In the Metriport codebase, during CommonWell SDK major version upgrades (like v5 to v6), it's expected and intentional to maintain both versions simultaneously - the new version (metriport/commonwell-sdk) and an aliased v1 version (metriport/commonwell-sdk-v1) for backward compatibility during migration periods.
Applied to files:
packages/lambdas/package.json
⏰ 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). (8)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/lambdas/package.json (1)
37-37: LGTM: CW SDK bumped to 6.3.0, v1 alias retained for back-compatThis aligns with the CW v2 migration plan and the dual v1/v2 strategy.
Comment on lines
+37
to
39
| "@metriport/commonwell-sdk": "6.3.0", | ||
| "@metriport/commonwell-sdk-v1": "npm:@metriport/commonwell-sdk@^5.9.11", | ||
| "@opensearch-project/opensearch": "^2.3.1", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pre-merge checks: publish, lockfile, and runtime compatibility
- Ensure 6.3.0 is published before merge (per release plan).
- Commit the updated lockfile to avoid CI drift.
- Confirm Lambda runtimes use Node 18+ if the SDK requires it; engines here still allow Node ≥16.
Run from repo root:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Verify package is published"
npm view @metriport/commonwell-sdk@6.3.0 version
echo "2) Check for mixed installs of v1/v2"
npm ls @metriport/commonwell-sdk @metriport/commonwell-sdk-v1 || true
echo "3) Find Lambda runtime declarations"
rg -nC2 -iP 'runtime:\s*nodejs(16|18|20)|AWS_LAMBDA_JS_RUNTIME' infra serverless cdk pulumi || true
echo "4) Show local engines field for lambdas"
jq '.engines' packages/lambdas/package.jsonLength of output: 873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "5) Check SDK engines requirement"
npm view @metriport/commonwell-sdk@6.3.0 engines
echo "3.1) Find runtime declarations across repository"
rg -nC2 -iP 'runtime\s*:\s*nodejs(16|18|20)' --glob '*' || true
rg -nC2 'AWS_LAMBDA_JS_RUNTIME' --glob '*' || trueLength of output: 367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Lockfiles present:"
ls -1 . | grep -E 'lock'
echo "Modified lockfiles:"
git status --porcelain | grep -E 'yarn.lock|package-lock.json' || echo "None"Length of output: 240
Commit updated lockfile
- Confirmed @metriport/commonwell-sdk@6.3.0 is published.
- package-lock.json wasn’t updated—run
npm installin packages/lambdas and commit the updated lockfile.
🤖 Prompt for AI Agents
In packages/lambdas/package.json around lines 37 to 39, the dependency change to
@metriport/commonwell-sdk@6.3.0 updated package.json but not package-lock.json;
run npm install from packages/lambdas to regenerate/update package-lock.json,
verify the lockfile includes @metriport/commonwell-sdk@6.3.0, run the test/build
to ensure nothing breaks, and commit/package the updated package-lock.json
alongside your package.json change.
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
Testing
Local
Staging
CW v1 - Confirm it still works:
CW v2:
CW v2 - extended
Release Plan
Summary by CodeRabbit
New Features
Improvements
Bug Fixes