ENG-513: doc downloader lambda uses CW v2 and v1 #4445
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
Part of ENG-513 Signed-off-by: RamilGaripov <ramil@metriport.com>
WalkthroughIntroduces request/response hooks in CommonWellOptions, adds a new CommonWellBase for shared Axios/TLS handling and transaction ID tracking, refactors CommonWell and CommonWellMember to extend the base, updates lambdas to select CommonWell v1 or v2 at runtime via a feature flag, and wires feature flags into Lambda environments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Caller
participant CWBase as CommonWellBase
participant Axios as Axios Instance
participant API as CommonWell API
Client->>CWBase: call api.request(config)
Note over CWBase: Request interceptor
CWBase->>Axios: preRequestHook(config)?
Axios->>API: HTTPS request
API-->>Axios: Response (headers: x-trace-id)
Note over CWBase: Response interceptor
Axios-->>CWBase: Response/Error
CWBase->>CWBase: Store lastTransactionId from headers
CWBase-->>Client: Response or throw Error
sequenceDiagram
autonumber
participant Handler as DocumentDownloader Lambda
participant FF as FeatureFlags (DynamoDB)
participant Secrets as Cert/Key Store
participant CWv1 as CommonWell v1
participant CWv2 as CommonWell v2
participant RunnerV1 as DocumentDownloaderLocal (v1)
participant RunnerV2 as DocumentDownloaderLocalV2 (v2)
Handler->>FF: isCommonwellV2EnabledForCx(cxId)
par Fetch credentials
Handler->>Secrets: get cwOrgCertificate
Handler->>Secrets: get cwOrgPrivateKey
end
alt V2 enabled
Handler->>CWv2: new CommonWell({...}, apiMode)
Handler->>RunnerV2: run(CWv2.api, query)
RunnerV2-->>Handler: result
else V2 disabled
Handler->>CWv1: new CommonWell({...})
Handler->>RunnerV1: run(CWv1.api, queryMeta)
RunnerV1-->>Handler: result
end
Handler-->>Handler: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/commonwell-sdk/src/client/commonwell-member.ts (2)
86-101: Encode all path segments used in URLs (memberId, id, thumbprint, purpose).Avoid malformed requests and potential path traversal by encoding dynamic segments. Apply this diff:
- `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org`, @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/`, @@ - const resp = await this.api.get(`${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org`, { + const resp = await this.api.get(`${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org`, { @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/`, @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/certificate`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/certificate`, @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/certificate`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/certificate`, @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/certificate/${thumbprint}/purpose/${purpose}`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/certificate/${encodeURIComponent(thumbprint)}/purpose/${encodeURIComponent(purpose)}`, @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/certificate`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/certificate`, @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/certificate/${thumbprint}`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/certificate/${encodeURIComponent(thumbprint)}`, @@ - `${CommonWellMember.MEMBER_ENDPOINT}/${this.memberId}/org/${id}/certificate/${thumbprint}/purpose/${purpose}`, + `${CommonWellMember.MEMBER_ENDPOINT}/${encodeURIComponent(this.memberId)}/org/${encodeURIComponent(id)}/certificate/${encodeURIComponent(thumbprint)}/purpose/${encodeURIComponent(purpose)}`,Also applies to: 111-127, 141-160, 171-190, 202-222, 234-254, 267-283, 295-311, 324-341, 353-369
182-186: Don’t rely on http-status “classes”; use a simple range check for 2xx.Prevents subtle breakage if the library API changes or isn’t present. Safer and clearer.
- if (httpStatus[`${status}_CLASS`] === httpStatus.classes.SUCCESSFUL) { + if (status >= 200 && status < 300) { return organizationSchema.parse(resp.data); }
🧹 Nitpick comments (7)
packages/lambdas/package.json (1)
35-37: Alpha versions published; optional engine alignment
- Confirmed
@metriport/api-sdk@18.0.0-alpha.3and@metriport/commonwell-sdk@6.0.2-alpha.1exist on NPM (npm view returned matching versions).- Optional: add an
"engines": { "node": ">=18" }field inpackages/lambdas/package.jsonto reflect the AWS Lambda Node.js 18+ runtime.packages/commonwell-sdk/src/client/common.ts (1)
29-29: Clarify JSDoc wording“after the request is sent” → “after a response is received”.
- /** Function to run after the request is sent. */ + /** Function to run after a response is received. */packages/lambdas/src/document-downloader.ts (1)
49-53: Remove unsafe type assertions on getSecret()
getSecret()may resolve tostring | undefined. You already validate after the await; keep the actual type to avoid masking issues.-const [cwOrgCertificate, cwOrgPrivateKey, isV2Enabled] = await Promise.all([ - getSecret(cwOrgCertificateSecret) as Promise<string>, - getSecret(cwOrgPrivateKeySecret) as Promise<string>, - isCommonwellV2EnabledForCx(cxId), -]); +const [cwOrgCertificate, cwOrgPrivateKey, isV2Enabled] = await Promise.all([ + getSecret(cwOrgCertificateSecret), + getSecret(cwOrgPrivateKeySecret), + isCommonwellV2EnabledForCx(cxId), +]);packages/commonwell-sdk/src/client/commonwell.ts (1)
173-176: Typo in error message“parametrs” → “parameters”.
- throw new Error("Programming error, 'id' is required when providing separated parametrs"); + throw new Error("Programming error, 'id' is required when providing separated parameters");packages/commonwell-sdk/src/client/commonwell-member.ts (1)
116-118: Endpoint consistency: trailing slashes vary.Some endpoints include a trailing slash while others don’t. Consider standardizing to avoid accidental 301/308s.
Also applies to: 175-177, 214-216, 246-248, 275-277, 303-305, 332-334, 361-363
packages/commonwell-sdk/src/client/commonwell-base.ts (2)
21-32: JSDoc parameters mention memberName/memberId but constructor doesn’t accept them.Clean up the docblock to avoid confusion.
- * @param memberName The name of the member. - * @param memberId The ID of the member (not the OID).
45-45: Enable HTTP keep-alive on the HTTPS agent.Improves Lambda performance and reduces TLS handshakes.
- this.httpsAgent = new Agent({ cert: orgCert, key: rsaPrivateKey }); + this.httpsAgent = new Agent({ cert: orgCert, key: rsaPrivateKey, keepAlive: true });
📜 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 (7)
packages/commonwell-sdk/src/client/common.ts(2 hunks)packages/commonwell-sdk/src/client/commonwell-base.ts(1 hunks)packages/commonwell-sdk/src/client/commonwell-member.ts(4 hunks)packages/commonwell-sdk/src/client/commonwell.ts(3 hunks)packages/infra/lib/lambdas-nested-stack.ts(5 hunks)packages/lambdas/package.json(1 hunks)packages/lambdas/src/document-downloader.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Don’t use null inside the app, only on code interacting with external interfaces/services, like DB and HTTP; convert to undefined before sending inwards into the code
Useconstwhenever possible
Useasync/awaitinstead of.then()
Naming: classes, enums: PascalCase
Naming: constants, variables, functions: camelCase
Naming: file names: kebab-case
Naming: Don’t use negative names, likenotEnabled, preferisDisabled
If possible, use decomposing objects for function parameters
Prefer Nullish Coalesce (??) than the OR operator (||) when you want to provide a default value
Avoid creating arrow functions
Use truthy syntax instead ofin- i.e.,if (data.link)notif ('link' in data)
While handling errors, keep the stack trace around: if you create a new Error (e.g., MetriportError), make sure to pass the original error as the new one’s cause so the stack trace is available upstream.
max column length is 100 chars
multi-line comments use/** */
top-level comments go after the import (save pre-import to basic file header, like license)
move literals to constants declared after imports when possible
Files:
packages/commonwell-sdk/src/client/commonwell-base.tspackages/commonwell-sdk/src/client/common.tspackages/infra/lib/lambdas-nested-stack.tspackages/commonwell-sdk/src/client/commonwell-member.tspackages/lambdas/src/document-downloader.tspackages/commonwell-sdk/src/client/commonwell.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use types whenever possible
Files:
packages/commonwell-sdk/src/client/commonwell-base.tspackages/commonwell-sdk/src/client/common.tspackages/infra/lib/lambdas-nested-stack.tspackages/commonwell-sdk/src/client/commonwell-member.tspackages/lambdas/src/document-downloader.tspackages/commonwell-sdk/src/client/commonwell.ts
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- U...
Files:
packages/commonwell-sdk/src/client/commonwell-base.tspackages/commonwell-sdk/src/client/common.tspackages/infra/lib/lambdas-nested-stack.tspackages/commonwell-sdk/src/client/commonwell-member.tspackages/lambdas/src/document-downloader.tspackages/commonwell-sdk/src/client/commonwell.ts
🧠 Learnings (14)
📓 Common learnings
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-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-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-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
📚 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-base.tspackages/commonwell-sdk/src/client/commonwell-member.tspackages/lambdas/src/document-downloader.tspackages/commonwell-sdk/src/client/commonwell.ts
📚 Learning: 2025-08-26T23:48:06.638Z
Learnt from: RadmirGari
PR: metriport/metriport#4448
File: packages/infra/lib/lambdas-nested-stack.ts:1049-1054
Timestamp: 2025-08-26T23:48:06.638Z
Learning: In packages/infra/lib/lambdas-nested-stack.ts, the API_URL environment variable for HL7 roster upload lambdas is correctly set to config.loadBalancerDnsName without needing an explicit https:// scheme prefix. The configuration works as intended.
Applied to files:
packages/infra/lib/lambdas-nested-stack.ts
📚 Learning: 2025-08-21T20:18:02.196Z
Learnt from: RamilGaripov
PR: metriport/metriport#4436
File: packages/core/src/command/feature-flags/types.ts:45-46
Timestamp: 2025-08-21T20:18:02.196Z
Learning: In the Metriport feature flags system, new schema keys in packages/core/src/command/feature-flags/types.ts are handled for backward compatibility through the initialFeatureFlags pattern in ffs-on-dynamodb.ts rather than adding schema-level defaults with .default(). This maintains consistency across all feature flag handling.
Applied to files:
packages/infra/lib/lambdas-nested-stack.ts
📚 Learning: 2025-04-01T20:57:29.282Z
Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from packages/api/src/aws/app-config.ts to packages/core/src/command/feature-flags/domain-ffs.ts.
Applied to files:
packages/infra/lib/lambdas-nested-stack.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-member.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-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-25T17:52:19.445Z
Learnt from: RamilGaripov
PR: metriport/metriport#4412
File: packages/api/src/external/commonwell-v2/command/organization/organization.ts:89-98
Timestamp: 2025-08-25T17:52:19.445Z
Learning: In packages/api/src/external/commonwell-v2/command/organization/organization.ts, when setting organizationId, homeCommunityId, and patientIdAssignAuthority fields for CommonWell v2 SDK, raw OID values (org.oid) can be used directly without the "urn:oid:" prefix - the SDK handles any necessary formatting internally.
Applied to files:
packages/commonwell-sdk/src/client/commonwell.ts
🧬 Code graph analysis (4)
packages/commonwell-sdk/src/client/commonwell-base.ts (1)
packages/commonwell-sdk/src/client/common.ts (2)
CommonWellOptions(21-30)DEFAULT_AXIOS_TIMEOUT_SECONDS(4-4)
packages/commonwell-sdk/src/client/commonwell-member.ts (3)
packages/commonwell-sdk/src/index.ts (2)
CommonWellMember(4-4)CommonWellMemberAPI(5-5)packages/commonwell-sdk/src/client/commonwell-base.ts (1)
CommonWellBase(12-97)packages/commonwell-sdk/src/client/commonwell-member-api.ts (1)
CommonWellMemberAPI(11-57)
packages/lambdas/src/document-downloader.ts (3)
packages/core/src/command/feature-flags/domain-ffs.ts (1)
isCommonwellV2EnabledForCx(323-326)packages/core/src/external/commonwell-v1/document/document-downloader-local.ts (1)
DocumentDownloaderLocal(36-320)packages/core/src/external/commonwell-v2/document/document-downloader-local-v2.ts (1)
DocumentDownloaderLocalV2(30-329)
packages/commonwell-sdk/src/client/commonwell.ts (2)
packages/commonwell-sdk/src/client/commonwell-base.ts (1)
CommonWellBase(12-97)packages/commonwell-sdk/src/client/commonwell-api.ts (1)
CommonWellAPI(76-134)
🔇 Additional comments (5)
packages/infra/lib/lambdas-nested-stack.ts (1)
150-151: Feature flags wiring for DocumentDownloader looks correctEnv var
FEATURE_FLAGS_TABLE_NAMEis set and read permissions are granted to the lambda. This matches the runtime use ofisCommonwellV2EnabledForCx.Also applies to: 406-407, 418-418, 435-436, 458-459
packages/lambdas/src/document-downloader.ts (2)
62-86: V1 fallback path looks soundQuery meta creation and downloader wiring match existing behavior. No issues spotted.
112-121: Local helper visibility change is fineMaking
makeCommonWellAPInon-exported and returning the v1 interface keeps the public surface minimal.packages/commonwell-sdk/src/client/commonwell.ts (1)
91-103: HTTP base refactor and 500-retry handling look correctDelegation to CommonWellBase and conditional retries on 500s are implemented cleanly.
Also applies to: 551-562
packages/commonwell-sdk/src/client/commonwell-member.ts (1)
187-189: Confirm temporary ENG-668 behavior is still desired.
getOneOrgreturnsundefinedfor any non-404 non-2xx. If the upstream has stabilized, consider reverting to throwing aMetriportError.Would you like me to open a follow-up to flip this once ENG-668 is resolved?
Comment on lines
+26
to
+29
| /** Function to run before the request is sent. */ | ||
| preRequestHook?: (request: InternalAxiosRequestConfig) => void; | ||
| /** Function to run after the request is sent. */ | ||
| postRequestHook?: (response: AxiosResponse) => void; |
There was a problem hiding this comment.
postRequestHook is defined but not invoked anywhere
postRequestHook is never called by the HTTP client (see CommonWellBase). Either wire it into the response interceptors or drop it to avoid dead config.
Suggested change in CommonWellBase (outside this file) to invoke the hook on both success and error:
- this.api.interceptors.response.use(
- this.axiosSuccessfulResponse(this),
- this.axiosErrorResponse(this)
- );
+ this.api.interceptors.response.use(
+ this.axiosSuccessfulResponse(this, options.postRequestHook),
+ this.axiosErrorResponse(this, options.postRequestHook)
+ );
@@
- private axiosSuccessfulResponse(_this: CommonWellBase) {
- return (response: AxiosResponse): AxiosResponse => {
+ private axiosSuccessfulResponse(
+ _this: CommonWellBase,
+ postRequestHook?: CommonWellOptions["postRequestHook"]
+ ) {
+ return (response: AxiosResponse): AxiosResponse => {
_this && _this.postRequest(response);
+ postRequestHook?.(response);
return response;
};
}
- private axiosErrorResponse(_this: CommonWellBase) {
+ private axiosErrorResponse(
+ _this: CommonWellBase,
+ postRequestHook?: CommonWellOptions["postRequestHook"]
+ ) {
//eslint-disable-next-line @typescript-eslint/no-explicit-any
- return (error: any): AxiosResponse => {
+ return (error: any): AxiosResponse => {
_this && _this.postRequest(error.response);
+ if (error?.response) postRequestHook?.(error.response);
throw error;
};
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/commonwell-sdk/src/client/common.ts around lines 26 to 29,
postRequestHook is declared but never invoked by the HTTP client; modify the
HTTP client (CommonWellBase) to call postRequestHook with the AxiosResponse on
successful responses and also call it (with the error response if available) in
the response error handler so the hook runs for both success and failure; ensure
you check for hook existence before calling and preserve existing error
propagation (rethrow) after invoking the hook.
Comment on lines
+54
to
+59
| options.preRequestHook && | ||
| this.api.interceptors.request.use(this.axiosPreRequest(this, options.preRequestHook)); | ||
| this.api.interceptors.response.use( | ||
| this.axiosSuccessfulResponse(this), | ||
| this.axiosErrorResponse(this) | ||
| ); |
There was a problem hiding this comment.
Wire postRequestHook and fix error interceptor return type.
Currently postRequestHook is unused, and the error interceptor declares an incorrect return type. Use the hook on both success/error paths and Promise.reject for errors.
- options.preRequestHook &&
- this.api.interceptors.request.use(this.axiosPreRequest(this, options.preRequestHook));
- this.api.interceptors.response.use(
- this.axiosSuccessfulResponse(this),
- this.axiosErrorResponse(this)
- );
+ if (options.preRequestHook) {
+ this.api.interceptors.request.use(this.axiosPreRequest(this, options.preRequestHook));
+ }
+ this.api.interceptors.response.use(
+ this.axiosSuccessfulResponse(this, options.postRequestHook),
+ this.axiosErrorResponse(this, options.postRequestHook)
+ );
@@
- private axiosSuccessfulResponse(_this: CommonWellBase) {
- return (response: AxiosResponse): AxiosResponse => {
- _this && _this.postRequest(response);
- return response;
- };
- }
- private axiosErrorResponse(_this: CommonWellBase) {
- //eslint-disable-next-line @typescript-eslint/no-explicit-any
- return (error: any): AxiosResponse => {
- _this && _this.postRequest(error.response);
- throw error;
- };
- }
+ private axiosSuccessfulResponse(
+ _this: CommonWellBase,
+ postRequestHook?: CommonWellOptions["postRequestHook"]
+ ) {
+ return (response: AxiosResponse): AxiosResponse => {
+ if (_this) _this.postRequest(response);
+ postRequestHook?.(response);
+ return response;
+ };
+ }
+ private axiosErrorResponse(
+ _this: CommonWellBase,
+ postRequestHook?: CommonWellOptions["postRequestHook"]
+ ) {
+ //eslint-disable-next-line @typescript-eslint/no-explicit-any
+ return (error: any) => {
+ const resp = error?.response as AxiosResponse | undefined;
+ if (_this && resp) _this.postRequest(resp);
+ if (resp) postRequestHook?.(resp);
+ return Promise.reject(error);
+ };
+ }Also applies to: 84-96
🤖 Prompt for AI Agents
In packages/commonwell-sdk/src/client/commonwell-base.ts around lines 54-59 (and
also apply same change to lines 84-96), the postRequestHook is never called and
the error interceptor returns the wrong type; update the response interceptors
so both the success and error handlers invoke options.postRequestHook (if
provided) with the request/response context, and change the error handler to
return Promise.reject(error) instead of returning the error value; ensure you
pass the instance/context and the response or error to the hook and that the
error path re-throws via Promise.reject to preserve the rejected promise chain.
Comment on lines
+88
to
98
| // V2 | ||
| const commonWell = new CommonWell({ | ||
| orgCert: cwOrgCertificateSecret, | ||
| rsaPrivateKey: cwOrgPrivateKeySecret, | ||
| orgName, | ||
| addOidPrefix(orgOid) | ||
| ); | ||
| const queryMeta = organizationQueryMeta(orgName, { npi: npi }); | ||
| oid: orgOid, | ||
| homeCommunityId: orgOid, | ||
| npi, | ||
| apiMode, | ||
| }); | ||
|
|
There was a problem hiding this comment.
V2 path passes secret ARNs instead of secret values; also missing client timeout
You fetched the cert/key values into cwOrgCertificate/cwOrgPrivateKey, but the V2 constructor uses the env names/ARNs. Also, V1 sets a 4‑minute Axios timeout; V2 falls back to the 120s default. Pass the resolved secrets and align the timeout.
- const commonWell = new CommonWell({
- orgCert: cwOrgCertificateSecret,
- rsaPrivateKey: cwOrgPrivateKeySecret,
+ const commonWell = new CommonWell({
+ orgCert: cwOrgCertificate,
+ rsaPrivateKey: cwOrgPrivateKey,
orgName,
oid: orgOid,
homeCommunityId: orgOid,
npi,
apiMode,
+ options: { timeout: timeout.asMilliseconds() },
});
@@
- const docDownloader = new DocumentDownloaderLocalV2({
+ const docDownloader = new DocumentDownloaderLocalV2({
region,
bucketName,
commonWell: { api: commonWell },
capture,
});Also applies to: 99-104
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
commonwell-sdklayoutTesting
Release Plan
Summary by CodeRabbit