ENG-532 Don't include req headers on CW errors #4092
Conversation
WalkthroughThis update primarily consists of version bumps to alpha releases across multiple package.json files in the repository, along with minor dependency version updates and reordering of devDependencies. Additionally, there are small internal refactors and error reporting adjustments in the CommonWell document query logic, but no changes to public APIs or exported entities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentQuery
participant ErrorCapture
Client->>DocumentQuery: Initiate document query
alt Error occurs during query
DocumentQuery->>ErrorCapture: capture.error(errorToString(error), { patientId, url })
end
DocumentQuery-->>Client: Return result or error
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (15)
⏰ Context from checks skipped due to timeout of 90000ms (14)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
972fc59 to
be0b530
Compare
| @@ -22,7 +22,7 @@ async function initQuery( | |||
| throw new Error(`Could not determine subject ID for document query`); | |||
| } | |||
| const url = `${CommonWell.DOCUMENT_QUERY_ENDPOINT}?subject.id=${subjectId}`; | |||
| const additionalInfo = { headers, patientId }; | |||
| const additionalInfo = { patientId, url }; | |||
There was a problem hiding this comment.
The main change to prevent the headers on Errors/Sentry
packages/eslint-rules/package.json
Outdated
There was a problem hiding this comment.
auto-formatted by lerna, it was not formatted properly before
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/commonwell-sdk/src/client/document.ts (1)
43-47: Consider removing headers from all error contexts for consistency.While the
initQueryfunction no longer includes headers in error context, thequery,queryFull, andretrievefunctions still include headers in their error handling. For consistency with the PR objective of not including request headers on CW errors, consider removing headers from all error contexts in this file.Apply this pattern to maintain consistency:
throw new CommonwellError(`Error parsing document query response`, err, { - headers, patientId, response, });throw new CommonwellError(`Error retrieve document`, err, { - headers, inputUrl, outputStream: outputStream ? "[object]" : outputStream, });Also applies to: 60-64, 82-86
♻️ Duplicate comments (1)
packages/commonwell-sdk/src/client/document.ts (1)
25-25: Headers removed from error context improves security.This change prevents potentially sensitive request headers from being logged to CloudWatch/Sentry while maintaining debugging context through the URL and patient ID.
🧹 Nitpick comments (6)
packages/api/package.json (1)
3-3: API package pre-release bump looks fineJust the version field updated. Because this package is consumed by external clients, be sure to:
- Publish with the
nexttag, notlatest.- Update release notes to call out the CloudWatch header-stripping change, as that is the semantic change users will notice.
packages/commonwell-cert-runner/package.json (1)
45-45: Caret range with pre-release can unintentionally resolve to the final GA
"@metriport/commonwell-sdk": "^5.9.10-alpha.0"allows npm to pick up any5.xincluding stable5.9.10once published.
If you want to stay on alpha only until it is validated, lock it exactly:- "@metriport/commonwell-sdk": "^5.9.10-alpha.0", + "@metriport/commonwell-sdk": "5.9.10-alpha.0",packages/commonwell-sdk/package.json (1)
63-63: Propagate shared alpha dependency preciselySame note as above – a caret on a prerelease may float to an unwanted stable version. Consider pinning:
- "@metriport/shared": "^0.23.10-alpha.0", + "@metriport/shared": "0.23.10-alpha.0",packages/ihe-gateway-sdk/package.json (1)
56-56: Lock shared prerelease to avoid accidental GA upgradeSame caret-range caution applies here.
- "@metriport/shared": "^0.23.10-alpha.0", + "@metriport/shared": "0.23.10-alpha.0",packages/shared/package.json (2)
106-106: devDependencies reorder is harmless but breaks alphabetical intentMoving
@metriport/eslint-rulesupward is stylistic. Consider addingsort-package-jsonor a lint-rule to enforce ordering automatically to avoid noisy diffs.
117-117: Unify TypeScript compiler version across packagesStill pinned to
4.9.5while some packages now use5.6.3. Decide on one compiler level repo-wide to prevent subtle type-level incompatibilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
package.json(2 hunks)packages/api-sdk/package.json(2 hunks)packages/api/package.json(1 hunks)packages/api/src/external/commonwell/document/document-query.ts(9 hunks)packages/carequality-cert-runner/package.json(2 hunks)packages/carequality-sdk/package.json(1 hunks)packages/commonwell-cert-runner/package.json(2 hunks)packages/commonwell-jwt-maker/package.json(2 hunks)packages/commonwell-sdk/package.json(2 hunks)packages/commonwell-sdk/src/client/document.ts(1 hunks)packages/core/package.json(2 hunks)packages/eslint-rules/package.json(1 hunks)packages/ihe-gateway-sdk/package.json(2 hunks)packages/infra/package.json(1 hunks)packages/mllp-server/package.json(2 hunks)packages/shared/package.json(3 hunks)packages/utils/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
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
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/carequality-sdk/package.jsonpackages/infra/package.jsonpackages/api/package.jsonpackage.jsonpackages/utils/package.jsonpackages/mllp-server/package.jsonpackages/commonwell-sdk/package.jsonpackages/commonwell-jwt-maker/package.jsonpackages/commonwell-cert-runner/package.jsonpackages/commonwell-sdk/src/client/document.tspackages/ihe-gateway-sdk/package.jsonpackages/carequality-cert-runner/package.jsonpackages/eslint-rules/package.jsonpackages/core/package.jsonpackages/api-sdk/package.jsonpackages/shared/package.jsonpackages/api/src/external/commonwell/document/document-query.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try ...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/commonwell-sdk/src/client/document.tspackages/api/src/external/commonwell/document/document-query.ts
🧠 Learnings (5)
packages/commonwell-sdk/package.json (1)
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.
packages/commonwell-jwt-maker/package.json (1)
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.
packages/ihe-gateway-sdk/package.json (1)
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.
packages/carequality-cert-runner/package.json (1)
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.
packages/api-sdk/package.json (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- 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: 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 (26)
packages/infra/package.json (1)
3-3: Version bump looks correct, but double-check changelog & publish tag
1.22.10-alpha.0fits the repo-wide alpha cadence.
Make sure:
CHANGELOG.md(or equivalent) reflects the bump.lerna publish/ npm publish uses thenexttag, or the intended pre-release tag, so consumers don’t accidentally pull an alpha.packages/carequality-sdk/package.json (1)
3-3: SDK version increment acknowledgedNo functional changes in this file—only
1.6.10-alpha.0.
Ensure generated artifacts (dist/) are rebuilt before publishing so that the registry contains the correct versioned code.packages/utils/package.json (1)
3-3: Utils package bumped to alpha – OKNothing else changed. Confirm that any downstream packages depending on
utilsvia semver range or workspace reference remain satisfied.package.json (1)
41-55: Workspace re-ordering is harmless but confirm CI scriptsMoving
packages/eslint-rulesto the top and addingpackages/utilslower in the list shouldn’t affect npm, but some custom scripts iterate overworkspacesin order. Quickly sanity-run yourlerna publish/ build pipeline to ensure the new ordering doesn’t break any assumptions.packages/mllp-server/package.json (1)
3-3: Ensure the alpha tag matches your release intent
0.3.10-alpha.0is a pre-release. Because"private": true, it will not publish to NPM, but CI/CD pipelines may still push the tag to your internal registry. Double-check that this bump was intended for an internal alpha cut and that any downstream packages expecting0.3.xhave their ranges updated.packages/commonwell-cert-runner/package.json (1)
3-3: Alpha bump looks fine – remember to publish before merge
1.26.12-alpha.0complies with the internal rule that certification-runner bumps follow CommonWell SDK bumps. Just make sure the alpha tarball actually lands in the registry so CI on dependent repos can resolve it.packages/commonwell-sdk/package.json (1)
3-3: Version bump acknowledged
5.9.10-alpha.0keeps semantic ordering and matches dependent packages. No functional concerns here.packages/ihe-gateway-sdk/package.json (1)
3-3: Alpha tag bump confirmed
0.19.10-alpha.0aligns with the shared package bump; LGTM.packages/shared/package.json (1)
3-3: Shared package alpha bump noted
0.23.10-alpha.0is consistent with downstream updates; no issues.packages/commonwell-jwt-maker/package.json (2)
3-3: Alpha version bump follows established SDK release process.The version bump to alpha aligns with the requirement to publish alpha versions before merging SDK changes.
44-44: Dependency version update is consistent with coordinated release.The update to the alpha version of
@metriport/commonwell-sdkmaintains consistency across the monorepo packages.packages/carequality-cert-runner/package.json (2)
3-3: Alpha version bump is appropriate for coordinated release.The version increment to alpha follows the established pattern for SDK package releases.
44-45: Dependency updates maintain version consistency.Both
@metriport/ihe-gateway-sdkand@metriport/sharedare updated to their respective alpha versions, ensuring coordinated release across packages.packages/core/package.json (2)
3-3: Alpha version update follows coordinated release pattern.The version bump maintains consistency with other packages in this coordinated alpha release.
156-156: DevDependency reordering is cosmetic but acceptable.The reordering of
@metriport/eslint-rulesin devDependencies doesn't affect functionality and appears to be for consistency across packages.packages/api-sdk/package.json (2)
3-3: SDK alpha version follows required release process.The alpha version bump is necessary for SDK packages as per established development workflow.
61-62: Alpha dependency updates maintain package coherence.The updates to
@metriport/commonwell-sdkand@metriport/sharedalpha versions ensure all related packages are released together.packages/eslint-rules/package.json (1)
2-30: LGTM: Version bump and formatting improvements.The alpha version bump is appropriate for pre-release, and the expanded JSON formatting improves readability. No functional changes detected.
packages/api/src/external/commonwell/document/document-query.ts (8)
10-10: LGTM: Import reordering.The import reordering doesn't affect functionality and may improve organization.
270-270: Excellent: Improved error reporting consistency.The change from
capture.messagetocapture.errorand the use oferrorToStringprovides better error handling and consistent formatting across the codebase.Also applies to: 279-279
304-312: Great refactoring: Named function declaration.Converting the inline arrow function to a named function declaration improves readability and follows the coding guidelines preference to avoid creating arrow functions.
391-391: Consistent error handling improvement.The change from
capture.messagetocapture.errormaintains consistency with the error reporting pattern used throughout the file.
527-531: Good refactoring: Named async function.Converting the inline conditional to a named async function improves readability and makes the code more maintainable.
555-557: Improved readability with named function.The conversion from inline arrow function to named function declaration enhances code clarity and follows the coding guidelines.
643-643: Consistent error formatting improvements.The addition of
errorToString(error)calls and the change fromcapture.messagetocapture.errorensures consistent error handling and formatting throughout the file.Also applies to: 749-749, 755-755
837-839: Good refactoring: Named function declaration.Converting the inline arrow function to a named function declaration improves code readability and follows the coding guidelines preference for named functions.
| "@metriport/eslint-rules": "file:packages/eslint-rules", | ||
| "@sentry/node": "^9.1.0", | ||
| "@tsconfig/node18": "^18.2.4", | ||
| "@types/node": "^22.13.5", | ||
| "ts-node": "^10.9.2", | ||
| "typescript": "^5.6.3", | ||
| "@metriport/eslint-rules": "file:packages/eslint-rules" | ||
| "typescript": "^5.6.3" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TypeScript version drifts from the rest of the monorepo
Most SDK packages still pin typescript to ^4.9.5, whereas this package is now on ^5.6.3. Mixing TS majors inside the same repo can lead to mismatched lib .d.ts files and build artefacts.
If you don’t actually rely on TS 5 features here, keep the repo consistent:
- "typescript": "^5.6.3"
+ "typescript": "^4.9.5"(or upgrade all packages together).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@metriport/eslint-rules": "file:packages/eslint-rules", | |
| "@sentry/node": "^9.1.0", | |
| "@tsconfig/node18": "^18.2.4", | |
| "@types/node": "^22.13.5", | |
| "ts-node": "^10.9.2", | |
| "typescript": "^5.6.3", | |
| "@metriport/eslint-rules": "file:packages/eslint-rules" | |
| "typescript": "^5.6.3" | |
| } | |
| "@metriport/eslint-rules": "file:packages/eslint-rules", | |
| "@sentry/node": "^9.1.0", | |
| "@tsconfig/node18": "^18.2.4", | |
| "@types/node": "^22.13.5", | |
| "ts-node": "^10.9.2", | |
| "typescript": "^4.9.5" | |
| } |
🤖 Prompt for AI Agents
In packages/mllp-server/package.json around lines 39 to 45, the TypeScript
version is set to ^5.6.3, which differs from the rest of the monorepo that uses
^4.9.5. To maintain consistency and avoid build issues, change the TypeScript
version here to ^4.9.5 unless you specifically need features from TypeScript 5.
Alternatively, coordinate an upgrade of TypeScript across all packages
simultaneously.
There was a problem hiding this comment.
out of scope for this PR
There was a problem hiding this comment.
@leite08 Understood! You're absolutely right that TypeScript version consistency is outside the scope of this PR focused on CloudWatch error handling.
Would you like me to create a follow-up issue to track the TypeScript version drift across the monorepo? This would help ensure it gets addressed in a future PR dedicated to dependency management.
Ref eng-532 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-532 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref: #000 - api@1.27.10-alpha.0 - @metriport/api-sdk@15.0.7-alpha.0 - @metriport/carequality-cert-runner@1.18.10-alpha.0 - @metriport/carequality-sdk@1.6.10-alpha.0 - @metriport/commonwell-cert-runner@1.26.12-alpha.0 - @metriport/commonwell-jwt-maker@1.24.12-alpha.0 - @metriport/commonwell-sdk@5.9.10-alpha.0 - @metriport/core@1.24.10-alpha.0 - @metriport/eslint-plugin-eslint-rules@1.0.2-alpha.0 - @metriport/ihe-gateway-sdk@0.19.10-alpha.0 - infrastructure@1.22.10-alpha.0 - mllp-server@0.3.10-alpha.0 - @metriport/shared@0.23.10-alpha.0 - utils@1.25.10-alpha.0 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-532 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
3d464db to
d1c5c5a
Compare
Ref eng-532 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Dependencies
none
Description
Don't include req headers on CW errors.
Testing
Release Plan
Summary by CodeRabbit
New Features
Refactor
Chores