ENG-538 Create FHIR SDK for easier programmatic usage of FHIR bundles #4097
Conversation
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-000 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FhirBundleSdk
participant SmartResource
participant Bundle
User->>FhirBundleSdk: new FhirBundleSdk(bundle)
FhirBundleSdk->>Bundle: Validate bundle type and structure
FhirBundleSdk->>FhirBundleSdk: Index resources by id, fullUrl, type
User->>FhirBundleSdk: getResourceById(id)
FhirBundleSdk->>FhirBundleSdk: Lookup resource in index
FhirBundleSdk-->>User: SmartResource
User->>SmartResource: getSubject() (reference method)
SmartResource->>FhirBundleSdk: Resolve reference by id/fullUrl
FhirBundleSdk-->>SmartResource: Linked SmartResource or undefined
User->>FhirBundleSdk: lookForBrokenReferences()
FhirBundleSdk->>FhirBundleSdk: Validate all references
FhirBundleSdk-->>User: ValidationResult
User->>FhirBundleSdk: exportSubset(resourceIds)
FhirBundleSdk->>FhirBundleSdk: Filter indexed resources
FhirBundleSdk-->>User: New Bundle with subset
User->>FhirBundleSdk: getPatients()
FhirBundleSdk->>FhirBundleSdk: Return all Patient resources as SmartResources
FhirBundleSdk-->>User: SmartResource[]
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 selected for processing (3)
✨ 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 (
|
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
packages/fhir-sdk/README.md
Outdated
|
|
||
| All FHIR resource types are supported with corresponding `get{ResourceType}()` and `get{ResourceType}ById()` methods: | ||
|
|
||
| `Patient`, `Observation`, `Encounter`, `Practitioner`, `Organization`, `Location`, `DiagnosticReport`, `Condition`, `AllergyIntolerance`, `MedicationRequest`, `Procedure`, `CarePlan`, `Goal`, `Device`, `Specimen`, `ImagingStudy`, `DocumentReference`, `Binary`, `Composition`, `ClinicalImpression`, `RiskAssessment` |
There was a problem hiding this comment.
Why did we outline these in specific? Our docs have the full list of resources Metriport supports in the consolidated page
There was a problem hiding this comment.
garbage from llm, deleted
packages/fhir-sdk/README.md
Outdated
|
|
||
| ```typescript | ||
| const result = sdk.validateReferences(); | ||
| console.log(result.isValid); // boolean |
There was a problem hiding this comment.
How are we doing this? There's a bundle validator lib we can use if needed.
There was a problem hiding this comment.
addressed - via rename
packages/fhir-sdk/README.md
Outdated
| ```typescript | ||
| const result = sdk.validateReferences(); | ||
| console.log(result.isValid); // boolean | ||
| console.log(result.brokenReferences); // Array of broken references |
There was a problem hiding this comment.
addressed - via rename and better explainer above.
packages/fhir-sdk/README.md
Outdated
| @@ -0,0 +1,135 @@ | |||
| # @metriport/fhir-sdk | |||
|
|
|||
| TypeScript SDK for parsing, querying, and manipulating FHIR bundles with smart reference resolution. | |||
There was a problem hiding this comment.
What version of FHIR? We use R4
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (16)
packages/fhir-sdk/.gitignore (1)
1-5: LGTM with minor formatting fix needed.The .gitignore entries are appropriate for a TypeScript package. However, there's trailing whitespace on line 5 after
coverage/.-coverage/ +coverage/package.json (1)
54-55: LGTM! Workspace integration is correct.The new
packages/fhir-sdkworkspace entry is properly added and formatted consistently with existing entries.For better maintainability, consider organizing workspace entries alphabetically:
"workspaces": [ "packages/shared", "packages/api-sdk", "packages/ihe-gateway-sdk", "packages/commonwell-sdk", "packages/carequality-sdk", "packages/core", "packages/commonwell-cert-runner", "packages/commonwell-jwt-maker", "packages/carequality-cert-runner", "packages/api", "packages/mllp-server", + "packages/eslint-rules", + "packages/fhir-sdk", "packages/infra", - "packages/utils", - "packages/eslint-rules", - "packages/fhir-sdk" + "packages/utils" ],packages/fhir-sdk/.eslintignore (1)
1-3: LGTM with minor formatting fix needed.The .eslintignore entries are appropriate for excluding build outputs, dependencies, and declaration files from linting. However, there's trailing whitespace on line 3 after
*.d.ts.-*.d.ts +*.d.tspackages/fhir-sdk/LICENSE (1)
1-21: LGTM with minor formatting fix needed.The MIT License is appropriate for the package. However, there's trailing whitespace on line 21 after
SOFTWARE..-SOFTWARE. +SOFTWARE.packages/fhir-sdk/src/__tests__/env-setup.ts (1)
3-7: Consider simplifying the path resolution logic.The current path construction is complex and could be made more readable. Consider using a more explicit approach to handle the monorepo structure.
-const cwd = process.cwd(); -const paths = [cwd, ...(cwd.includes("packages") ? [] : ["packages", "fhir-sdk"])]; -// regular config so it can load .env if present -dotenv.config({ path: path.resolve(...paths, ".env") }); -dotenv.config({ path: path.resolve(...paths, ".env.test") }); +const cwd = process.cwd(); +const isInPackageDir = cwd.includes("packages"); +const basePath = isInPackageDir ? cwd : path.join(cwd, "packages", "fhir-sdk"); + +// Load environment files - regular config so it can load .env if present +dotenv.config({ path: path.resolve(basePath, ".env") }); +dotenv.config({ path: path.resolve(basePath, ".env.test") });packages/fhir-sdk/.eslintrc.js (1)
2-7: Fix formatting to align with coding standards.The configuration content is correct, but the formatting could be improved for consistency.
-module.exports = { - extends: ['eslint:recommended', 'plugin:@typescript-eslint/recommended', 'prettier', 'plugin:@metriport/eslint-rules/recommended'], - parser: '@typescript-eslint/parser', - plugins: ['@typescript-eslint', '@metriport/eslint-rules'], - root: true, - }; +module.exports = { + extends: [ + "eslint:recommended", + "plugin:@typescript-eslint/recommended", + "prettier", + "plugin:@metriport/eslint-rules/recommended", + ], + parser: "@typescript-eslint/parser", + plugins: ["@typescript-eslint", "@metriport/eslint-rules"], + root: true, +};packages/utils/src/fhir/fhir-deduplication/report/validate-references.ts (1)
47-293: Consider refactoring this large function for maintainability.While the function rename is good, the
findRefsfunction is quite large (246 lines) and handles many different resource types. Consider breaking it down into smaller, resource-type-specific functions to improve maintainability and testability.For example, you could extract resource-specific reference finding into separate functions:
function findRefsInDiagnosticReport(resource: DiagnosticReport): string[] { // Handle DiagnosticReport-specific reference patterns } function findRefsInEncounter(resource: Encounter): string[] { // Handle Encounter-specific reference patterns } function findRefs<T extends Resource>(resource: T): string[] { const refs: string[] = []; // Dispatch to resource-specific handlers switch (resource.resourceType) { case "DiagnosticReport": refs.push(...findRefsInDiagnosticReport(resource as DiagnosticReport)); break; case "Encounter": refs.push(...findRefsInEncounter(resource as Encounter)); break; // ... other cases } // Handle common reference patterns refs.push(...findCommonRefs(resource)); return refs; }packages/fhir-sdk/jest.config.ts (1)
5-6: Avoid mutating global env vars inside config – makeENV_TYPEinjectable
Settingprocess.env.ENV_TYPE = "dev"here means every Jest run (including CI) will always behave as “dev”, even when another stage might want to test prod–specific behaviour. Prefer passing the variable via thejestCLI (cross-env ENV_TYPE=dev jest …) or a dedicatedsetupEnv.tsso callers keep control.packages/utils/package.json (2)
39-41: Scripts added without description – consider grouping or prefixing
fhir-demoandtest-typingare helpful, but utils already has ~40 scripts. To avoid clutter and improve discoverability, prefix them (demo:fhir,test:typing) or document them inREADME.md.
83-83: Use workspace protocol for local dependency
Instead of"@metriport/fhir-sdk": "file:packages/fhir-sdk", leverage the npmworkspace:*protocol ("@metriport/fhir-sdk": "workspace:*"). It keeps versions in sync automatically and avoids accidental drift.packages/fhir-sdk/tsconfig.json (2)
8-9:emitDecoratorMetadata/experimentalDecoratorsenabled but unused
Neither the SDK source nor typings use decorators. Keeping these flags on enlarges the output JS and slows compilation. Disable them unless you plan to introduce decorators.
18-19: MissingrootDirmay emit deep relative paths
Without"rootDir": "./src",tscinfers it, sometimes resulting indist/src/**structures. Explicitly settingrootDiravoids that and makes the build layout deterministic."outDir": "./dist", + "rootDir": "./src",packages/fhir-sdk/README.md (2)
62-68: Typo – broken reference boolean description inverted
hasBrokenReferencesis described as “true if at least one broken reference is found, false otherwise”. The second clause is redundant. Suggested tweak for clarity:-console.log(result.hasBrokenReferences); // true if at least one broken reference is found, false otherwise +console.log(result.hasBrokenReferences); // true when the bundle contains broken references
120-124: Big-O claims need a qualifier
getByTypeis listed as O(n) where n = resources of that type. Implementation currently filters an array each call — that is O(totalResources) unless per-type indices are cached. Clarify or adjust implementation to match the stated complexity.packages/fhir-sdk/src/index.t A3D4 s (2)
1-1: File-level eslint disable seems overly broad.The file-level
eslint-disable @typescript-eslint/no-unused-varsmight hide actual unused variables. Consider removing this and using line-specific disables only where necessary.
465-469: Hardcoded array reference fields may be incomplete.The
isArrayReferenceFieldmethod uses a hardcoded set of field names that may not cover all array reference fields in FHIR resources. Consider making this more comprehensive or configurable.Would you like me to help identify additional array reference fields from the FHIR specification or implement a more robust solution?
📜 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 (24)
package.json(1 hunks)packages/fhir-sdk/.eslintignore(1 hunks)packages/fhir-sdk/.eslintrc.js(1 hunks)packages/fhir-sdk/.gitignore(1 hunks)packages/fhir-sdk/LICENSE(1 hunks)packages/fhir-sdk/README.md(1 hunks)packages/fhir-sdk/jest.config.ts(1 hunks)packages/fhir-sdk/package.json(1 hunks)packages/fhir-sdk/src/__tests__/env-setup.ts(1 hunks)packages/fhir-sdk/src/__tests__/fhir-bundle-sdk-basic.test.ts(1 hunks)packages/fhir-sdk/src/__tests__/fhir-bundle-sdk.test.ts(1 hunks)packages/fhir-sdk/src/__tests__/fixtures/fhir-bundles.ts(1 hunks)packages/fhir-sdk/src/__tests__/phase1-verification.test.ts(1 hunks)packages/fhir-sdk/src/__tests__/phase2-verification.test.ts(1 hunks)packages/fhir-sdk/src/__tests__/phase3-verification.test.ts(1 hunks)packages/fhir-sdk/src/__tests__/phase4-verification.test.ts(1 hunks)packages/fhir-sdk/src/__tests__/phase5-verification.test.ts(1 hunks)packages/fhir-sdk/src/index.ts(1 hunks)packages/fhir-sdk/src/types/smart-resources.ts(1 hunks)packages/fhir-sdk/tsconfig.json(1 hunks)packages/utils/package.json(2 hunks)packages/utils/src/fhir/fhir-deduplication/dedup-files.ts(2 hunks)packages/utils/src/fhir/fhir-deduplication/report/report-dedup.ts(2 hunks)packages/utils/src/fhir/fhir-deduplication/report/validate-references.ts(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:
package.jsonpackages/fhir-sdk/LICENSEpackages/utils/src/fhir/fhir-deduplication/report/report-dedup.tspackages/utils/src/fhir/fhir-deduplication/dedup-files.tspackages/fhir-sdk/src/__tests__/fhir-bundle-sdk-basic.test.tspackages/fhir-sdk/src/__tests__/env-setup.tspackages/fhir-sdk/README.mdpackages/fhir-sdk/tsconfig.jsonpackages/utils/package.jsonpackages/fhir-sdk/src/__tests__/phase4-verification.test.tspackages/fhir-sdk/src/__tests__/phase1-verification.test.tspackages/utils/src/fhir/fhir-deduplication/report/validate-references.tspackages/fhir-sdk/jest.config.tspackages/fhir-sdk/src/__tests__/phase2-verification.test.tspackages/fhir-sdk/package.jsonpackages/fhir-sdk/src/__tests__/phase5-verification.test.tspackages/fhir-sdk/src/__tests__/phase3-verification.test.tspackages/fhir-sdk/src/index.tspackages/fhir-sdk/src/__tests__/fixtures/fhir-bundles.tspackages/fhir-sdk/src/__tests__/fhir-bundle-sdk.test.tspackages/fhir-sdk/src/types/smart-resources.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/utils/src/fhir/fhir-deduplication/report/report-dedup.tspackages/utils/src/fhir/fhir-deduplication/dedup-files.tspackages/fhir-sdk/src/__tests__/fhir-bundle-sdk-basic.test.tspackages/fhir-sdk/src/__tests__/env-setup.tspackages/fhir-sdk/src/__tests__/phase4-verification.test.tspackages/fhir-sdk/src/__tests__/phase1-verification.test.tspackages/utils/src/fhir/fhir-deduplication/report/validate-references.tspackages/fhir-sdk/jest.config.tspackages/fhir-sdk/src/__tests__/phase2-verification.test.tspackages/fhir-sdk/src/__tests__/phase5-verification.test.tspackages/fhir-sdk/src/__tests__/phase3-verification.test.tspackages/fhir-sdk/src/index.tspackages/fhir-sdk/src/__tests__/fixtures/fhir-bundles.tspackages/fhir-sdk/src/__tests__/fhir-bundle-sdk.test.tspackages/fhir-sdk/src/types/smart-resources.ts
🧬 Code Graph Analysis (2)
packages/fhir-sdk/src/index.ts (2)
packages/fhir-sdk/src/types/smart-resources.ts (3)
Smart(43-43)isReferenceMethod(483-486)getReferenceField(491-494)packages/api/src/routes/medical/schemas/fhir.ts (1)
BundleEntry(22-22)
packages/fhir-sdk/src/types/smart-resources.ts (1)
packages/fhir-sdk/src/index.ts (25)
Smart(34-34)Resource(59-59)Patient(55-55)Location(47-47)Encounter(44-44)Practitioner(56-56)Organization(54-54)RelatedPerson(58-58)Observation(53-53)Medication(48-48)DiagnosticReport(42-42)AllergyIntolerance(37-37)Condition(40-40)Composition(39-39)Coverage(41-41)DocumentReference(43-43)Immunization(46-46)MedicationRequest(51-51)Procedure(57-57)FamilyMemberHistory(45-45)MedicationAdministration(49-49)MedicationDispense(50-50)MedicationStatement(52-52)RiskAssessment(60-60)ServiceRequest(61-61)
🪛 Biome (1.9.4)
packages/fhir-sdk/src/types/smart-resources.ts
[error] 85-87: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 303-305: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (17)
packages/utils/src/fhir/fhir-deduplication/report/report-dedup.ts (2)
33-33: LGTM! Improved function naming for clarity.The import name change from
validateReferencestolookForBrokenReferencesprovides better semantic clarity about the function's purpose.
147-147: LGTM! Function call updated consistently.The function call properly reflects the renamed import, maintaining consistency across the codebase.
packages/utils/src/fhir/fhir-deduplication/dedup-files.ts (2)
21-21: LGTM - Consistent function renaming.The import update aligns with the function rename in the validation module.
82-82: LGTM - Function call updated consistently.The function call correctly uses the renamed
lookForBrokenReferencesfunction, maintaining the same behavior.packages/fhir-sdk/src/__tests__/fhir-bundle-sdk-basic.test.ts (1)
8-32: Excellent test structure and coverage.The test suite follows TDD best practices with:
- Clear functional requirement mapping (FR-1.1, FR-1.2, FR-1.3)
- Proper test organization with nested describes
- Specific error message validation
- Good coverage of constructor validation scenarios
packages/utils/src/fhir/fhir-deduplication/report/validate-references.ts (1)
10-10: LGTM - Better function naming.The rename from
validateReferencestolookForBrokenReferencesis more descriptive and accurately reflects the function's behavior of searching for and reporting broken references.packages/fhir-sdk/jest.config.ts (1)
12-15:testMatchglob looks non-standard – verify it still catches all tests
The(*.)+part is uncommon and, depending on your shell/Jest version, may not expand as intended (e.g. it won’t matchfoo.test.tson some setups). Double-check withnpx jest --listTeststhat every intended file is picked up.packages/fhir-sdk/src/__tests__/phase1-verification.test.ts (1)
1-173: Excellent comprehensive test coverage for bundle initialization and resource retrieval.This test suite provides thorough validation of the core SDK functionality with proper TypeScript typing, performance requirements, and edge case handling. The test structure is well-organized and follows testing best practices.
packages/fhir-sdk/src/__tests__/phase4-verification.test.ts (1)
1-223: Well-structured and comprehensive export functionality tests.The test suite thoroughly covers all export methods with proper edge case handling, metadata preservation, and performance requirements. The test organization is excellent and follows the established patterns.
packages/fhir-sdk/src/__tests__/phase5-verification.test.ts (2)
1-1: Appropriate ESLint disable for test context.The ESLint disable for non-null assertion is justified in test files where we often need to assert on known test data.
2-250: Excellent coverage of smart reference resolution functionality.This test suite comprehensively validates the dynamic getter methods, reference chaining, caching behavior, and smart resource markers. The tests properly verify both the functional requirements and performance characteristics of the smart proxy implementation.
packages/fhir-sdk/src/__tests__/phase2-verification.test.ts (1)
1-283: Comprehensive and well-structured type-specific getter tests.This test suite provides excellent coverage of all type-specific resource getters with proper TypeScript typing, performance validation, memory management checks, and robust edge case handling. The test organization follows best practices and validates both functional and non-functional requirements.
packages/fhir-sdk/src/__tests__/fixtures/fhir-bundles.ts (1)
1-382: Well-structured test fixtures with comprehensive coverage.The test fixtures provide excellent coverage for testing the SDK functionality, including valid bundles, edge cases, and intentionally invalid data for error testing.
packages/fhir-sdk/src/__tests__/fhir-bundle-sdk.test.ts (1)
1-554: Comprehensive test coverage with clear functional requirement mapping.The test suite is well-structured with clear mapping to functional requirements (FR-1.1 through FR-9.4). The performance tests appropriately verify O(1) lookup times.
packages/fhir-sdk/src/index.ts (1)
138-282: Excellent metaprogramming approach for dynamic method generation.The
RESOURCE_METHODSconfiguration combined with the static initialization block is a clean implementation that reduces code duplication and ensures consistency across all resource type getters.packages/fhir-sdk/src/types/smart-resources.ts (2)
85-87: Consider using a type alias for empty interface.The static analysis tool suggests using a type alias instead of an empty interface. However, keeping it as an interface allows for future extension without breaking changes.
367-478: Well-designed reference field mapping with proper nested path support.The
REFERENCE_METHOD_MAPPINGcorrectly handles nested reference paths (e.g., "performer.actor") for resources like Immunization and Procedure, showing good understanding of FHIR resource structures.
| "exports": { | ||
| ".": "./dist/index.js" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete export map for ESM consumers
exports only exposes ./dist/index.js. TypeScript users relying on import { … } from "@metriport/fhir-sdk" will get types, but Node ESM consumers need "./dist/index.mjs" or "./dist/index.js" with "type": "module". Decide on CJS vs ESM, then expose both or set "type": "module".
🤖 Prompt for AI Agents
In packages/fhir-sdk/package.json around lines 17 to 19, the exports field only
exposes "./dist/index.js", which is incomplete for ESM consumers. Decide whether
the package will use CommonJS or ESM modules; if ESM, add "./dist/index.mjs" to
exports or set "type": "module" in package.json. Alternatively, expose both CJS
and ESM entry points in the exports map to support both module systems properly.
| "typesVersions": { | ||
| "*": { | ||
| "/": [ | ||
| "dist/index.d.ts" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
typesVersions pattern likely incorrect
The leading "/" key isn’t recognised by TypeScript. Standard pattern is "*": { "*": ["dist/index.d.ts"] }. Current config may break editor IntelliSense.
- "typesVersions": {
- "*": {
- "/": [
- "dist/index.d.ts"
- ]
- }
- },
+ "typesVersions": {
+ "*": {
+ "*": ["dist/index.d.ts"]
+ }
+ },📝 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.
| "typesVersions": { | |
| "*": { | |
| "/": [ | |
| "dist/index.d.ts" | |
| ] | |
| } | |
| }, | |
| "typesVersions": { | |
| "*": { | |
| "*": ["dist/index.d.ts"] | |
| } | |
| }, |
🤖 Prompt for AI Agents
In packages/fhir-sdk/package.json around lines 20 to 26, the typesVersions field
uses an incorrect pattern with a leading "/" key, which TypeScript does not
recognize and can break IntelliSense. Replace the current pattern with the
standard one by changing the key from "/" to "*" so it reads "typesVersions": {
"*": { "*": ["dist/index.d.ts"] } } to ensure proper type resolution.
| private canResolveReference(reference: string): boolean { | ||
| // Try to resolve by resource ID (e.g., "Patient/123") | ||
| if (reference.includes("/")) { | ||
| const [, resourceId] = reference.split("/"); | ||
| if (resourceId && this.resourcesById.has(resourceId)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // Try to resolve by fullUrl (e.g., "urn:uuid:123") | ||
| if (this.resourcesByFullUrl.has(reference)) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated reference resolution logic.
The canResolveReference method duplicates the resolution logic from resolveReferenceObject. Consider refactoring to reuse the resolution logic.
private canResolveReference(reference: string): boolean {
- // Try to resolve by resource ID (e.g., "Patient/123")
- if (reference.includes("/")) {
- const [, resourceId] = reference.split("/");
- if (resourceId && this.resourcesById.has(resourceId)) {
- return true;
- }
- }
-
- // Try to resolve by fullUrl (e.g., "urn:uuid:123")
- if (this.resourcesByFullUrl.has(reference)) {
- return true;
- }
-
- return false;
+ return this.resolveReferenceObject({ reference }) !== undefined;
}📝 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.
| private canResolveReference(reference: string): boolean { | |
| // Try to resolve by resource ID (e.g., "Patient/123") | |
| if (reference.includes("/")) { | |
| const [, resourceId] = reference.split("/"); | |
| if (resourceId && this.resourcesById.has(resourceId)) { | |
| return true; | |
| } | |
| } | |
| // Try to resolve by fullUrl (e.g., "urn:uuid:123") | |
| if (this.resourcesByFullUrl.has(reference)) { | |
| return true; | |
| } | |
| return false; | |
| } | |
| private canResolveReference(reference: string): boolean { | |
| return this.resolveReferenceObject({ reference }) !== undefined; | |
| } |
🤖 Prompt for AI Agents
In packages/fhir-sdk/src/index.ts around lines 513 to 528, the
canResolveReference method duplicates logic found in resolveReferenceObject for
checking if a reference can be resolved. Refactor canResolveReference to call
resolveReferenceObject and return true if it returns a valid resource, otherwise
false. This removes duplication and centralizes reference resolution logic in
one method.
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/fhir-sdk/src/index.ts (1)
512-527: Refactor duplicated reference resolution logic.The
canResolveReferencemethod duplicates the resolution logic fromresolveReferenceObject. Consider refactoring to reuse the resolution logic.private canResolveReference(reference: string): boolean { - // Try to resolve by resource ID (e.g., "Patient/123") - if (reference.includes("/")) { - const [, resourceId] = reference.split("/"); - if (resourceId && this.resourcesById.has(resourceId)) { - return true; - } - } - - // Try to resolve by fullUrl (e.g., "urn:uuid:123") - if (this.resourcesByFullUrl.has(reference)) { - return true; - } - - return false; + return this.resolveReferenceObject({ reference }) !== undefined; }
🧹 Nitpick comments (3)
packages/fhir-sdk/src/index.ts (3)
1-1: Remove or justify the ESLint disable comment.The blanket disable of
@typescript-eslint/no-unused-varsat the file level is not recommended. If this is needed for the dynamically generated methods, apply it only to those specific lines.-/* eslint-disable @typescript-eslint/no-unused-vars */
94-95: Use a more specific type instead of disabling ESLint.Instead of disabling the
no-explicit-anyrule, consider using a more specific type for the cache.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - private smartResourceArrayCache: Map<string, Smart<any>[]> = new Map(); + private smartResourceArrayCache: Map<string, Smart<Resource>[]> = new Map();
464-468: Consider making array reference fields configurable or more comprehensive.The hardcoded set of array reference fields may not cover all FHIR resource types. This could lead to incorrect behavior for resources with array references not in this list.
Consider either:
- Making this configurable via constructor options
- Using FHIR metadata/profiles to determine array fields dynamically
- Expanding the list to be more comprehensive
For now, you could add a comment documenting this limitation:
private isArrayReferenceField(fieldName: string, resourceType: string): boolean { // Known array reference fields + // TODO: This is not exhaustive and may need to be expanded based on FHIR profiles const arrayFields = new Set(["performer", "participant", "result", "generalPractitioner"]); return arrayFields.has(fieldName); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/fhir-sdk/src/index.ts(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/fhir-sdk/src/index.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/fhir-sdk/src/index.ts
🧬 Code Graph Analysis (1)
packages/fhir-sdk/src/index.ts (2)
packages/fhir-sdk/src/types/smart-resources.ts (3)
Smart(43-43)isReferenceMethod(483-486)getReferenceField(491-494)packages/api/src/routes/medical/schemas/fhir.ts (1)
BundleEntry(22-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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)
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/fhir-sdk/src/index.ts (3)
512-527: Refactor duplicated reference resolution logic.The
canResolveReferencemethod duplicates the resolution logic fromresolveReferenceObject. Consider refactoring to reuse the resolution logic.private canResolveReference(reference: string): boolean { - // Try to resolve by resource ID (e.g., "Patient/123") - if (reference.includes("/")) { - const [, resourceId] = reference.split("/"); - if (resourceId && this.resourcesById.has(resourceId)) { - return true; - } - } - - // Try to resolve by fullUrl (e.g., "urn:uuid:123") - if (this.resourcesByFullUrl.has(reference)) { - return true; - } - - return false; + return this.resolveReferenceObject({ reference }) !== undefined; }
562-565: Fix inverted boolean logic in validation result.The
hasBrokenReferencesfield should betruewhen broken references exist, but the current logic returnstruewhen there are no broken references.return { - hasBrokenReferences: brokenReferences.length === 0, + hasBrokenReferences: brokenReferences.length > 0, brokenReferences: brokenReferences, };
639-657: Remove duplicate method or clarify the distinction.The
getResourceByIdWithTypemethod appears to duplicate the functionality of the privategetResourceByIdAndTypemethod. Consider either removing one of them or having the public method call the private one.getResourceByIdWithType<T extends Resource>( id: string, resourceType: string ): Smart<T> | undefined { - // First try to find by resource.id - const resourceById = this.resourcesById.get(id); - if (resourceById && resourceById.resourceType === resourceType) { - return this.createSmartResource(resourceById) as unknown as Smart<T>; - } - - // Then try to find by fullUrl - const resourceByFullUrl = this.resourcesByFullUrl.get(id); - if (resourceByFullUrl && resourceByFullUrl.resourceType === resourceType) { - return this.createSmartResource(resourceByFullUrl) as unknown as Smart<T>; - } - - // Return undefined if not found or type doesn't match (FR-3.4) - return undefined; + return this.getResourceByIdAndType<T>(id, resourceType); }
🧹 Nitpick comments (1)
packages/fhir-sdk/src/index.ts (1)
1-1: Remove unnecessary eslint-disable comment.The imported types are actively used throughout the file, so this eslint-disable comment is not needed.
-/* eslint-disable @typescript-eslint/no-unused-vars */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/fhir-sdk/src/index.ts(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/fhir-sdk/src/index.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/fhir-sdk/src/index.ts
🧬 Code Graph Analysis (1)
packages/fhir-sdk/src/index.ts (2)
packages/fhir-sdk/src/types/smart-resources.ts (3)
Smart(43-43)isReferenceMethod(483-486)getReferenceField(491-494)packages/api/src/routes/medical/schemas/fhir.ts (1)
BundleEntry(22-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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)
| /** | ||
| * Check if a reference field expects an array of references | ||
| */ | ||
| private isArrayReferenceField(fieldName: string, resourceType: string): boolean { | ||
| // Known array reference fields | ||
| const arrayFields = new Set(["performer", "participant", "result", "generalPractitioner"]); | ||
| return arrayFields.has(fieldName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand the list of array reference fields or implement a more comprehensive solution.
The current hardcoded set of array fields is incomplete. Many FHIR resources have additional array reference fields not covered here (e.g., hasMember in Observation, episodeOfCare in Encounter).
Consider either:
- Expanding this list to be more comprehensive
- Implementing a resource-type-specific mapping
- Adding a comment documenting this limitation
Example of a more comprehensive approach:
private isArrayReferenceField(fieldName: string, resourceType: string): boolean {
- // Known array reference fields
- const arrayFields = new Set(["performer", "participant", "result", "generalPractitioner"]);
- return arrayFields.has(fieldName);
+ // Known array reference fields by resource type
+ const arrayFieldsByType: Record<string, Set<string>> = {
+ Observation: new Set(["performer", "hasMember", "derivedFrom"]),
+ Encounter: new Set(["participant", "episodeOfCare", "appointment"]),
+ Patient: new Set(["generalPractitioner"]),
+ DiagnosticReport: new Set(["performer", "result", "specimen"]),
+ // Add more resource-specific mappings as needed
+ };
+
+ const globalArrayFields = new Set(["performer", "participant"]);
+ const typeSpecificFields = arrayFieldsByType[resourceType];
+
+ return globalArrayFields.has(fieldName) ||
+ (typeSpecificFields?.has(fieldName) ?? false);
}📝 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.
| /** | |
| * Check if a reference field expects an array of references | |
| */ | |
| private isArrayReferenceField(fieldName: string, resourceType: string): boolean { | |
| // Known array reference fields | |
| const arrayFields = new Set(["performer", "participant", "result", "generalPractitioner"]); | |
| return arrayFields.has(fieldName); | |
| } | |
| /** | |
| * Check if a reference field expects an array of references | |
| */ | |
| private isArrayReferenceField(fieldName: string, resourceType: string): boolean { | |
| // Known array reference fields by resource type | |
| const arrayFieldsByType: Record<string, Set<string>> = { | |
| Observation: new Set(["performer", "hasMember", "derivedFrom"]), | |
| Encounter: new Set(["participant", "episodeOfCare", "appointment"]), | |
| Patient: new Set(["generalPractitioner"]), | |
| DiagnosticReport: new Set(["performer", "result", "specimen"]), | |
| // Add more resource-specific mappings as needed | |
| }; | |
| const globalArrayFields = new Set(["performer", "participant"]); | |
| const typeSpecificFields = arrayFieldsByType[resourceType]; | |
| return ( | |
| globalArrayFields.has(fieldName) || | |
| (typeSpecificFields?.has(fieldName) ?? false) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/fhir-sdk/src/index.ts around lines 461 to 468, the method
isArrayReferenceField uses a limited hardcoded set of array reference fields
which is incomplete. To fix this, expand the arrayFields set to include more
known array reference fields from various FHIR resources or implement a mapping
keyed by resourceType that returns the correct array fields for each resource.
Alternatively, add a clear comment explaining the current limitation and that
the list is not exhaustive.
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-538 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
Description
This PR creates a "FHIR SDK" - a library that Metriport can put its fhir bundles into in order to work with the data more easily, quickly accessing resources, types of resources, and walking references.
The full PR is ~800 LoC and 500 Lines of types.
The remaining is mainly tests and lockfile updates.
For more information, view README.md
Testing
Lots of unit tests.
Release Plan
Summary by CodeRabbit
New Features
Bug Fixes
Chores