ENG-403 Selectivelly load refs for consolidated search #3953
Conversation
|
""" WalkthroughThis set of changes refactors and extends the logic for hydrating missing FHIR resource references by grouping and filtering missing references by resource type. It updates typing and parameter names for clarity, introduces new test utilities and fixtures, restructures test coverage with parameterized tests, and modifies consolidated search to use a new consolidated data search approach with hydration and reverse hydration. Several utility and test functions were added or updated for consistency and improved test data handling. Minor fixes include typo corrections, enhanced reference formatting, and adding ingestion date fields for OpenSearch indexing. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant hydrateMissingReferences
participant groupByType
participant SearchService
TestSuite->>hydrateMissingReferences: Call with resources, searchService, iteration=1
hydrateMissingReferences->>groupByType: Group resources by type
hydrateMissingReferences->>SearchService: Fetch missing referenced resources filtered by type
SearchService-->>hydrateMissingReferences: Return hydrated resources
hydrateMissingReferences->>hydrateMissingReferences: Recursively call for missing references if iteration < max
hydrateMissingReferences-->>TestSuite: Return fully hydrated resources
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)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (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 (
|
4e19451 to
e18989a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/core/src/external/fhir/__tests__/medication-dispense.ts (1)
5-22: Field-precedence may unintentionally override caller-provided dataBecause
...paramsis spread last, anyextension,status, or evenmedicationReferencesupplied by the caller will silently replace the defaults defined earlier in the literal.If that is intentional → great.
If you want the defaults to win when the caller omits a field (and only then), swap the order:resourceType: "MedicationDispense", - ...(params?.id ? { id: params.id } : { id: faker.string.uuid() }), - extension: [ /* default */ ], - status: "completed", - medicationReference, - ...params, + ...(params?.id ? { id: params.id } : { id: faker.string.uuid() }), + ...params, + extension: params?.extension ?? [ + { + url: "https://public.metriport.com/fhir/StructureDefinition/doc-id-extension.json", + valueString: `${faker.string.uuid()}.xml`, + }, + ], + status: params?.status ?? "completed", + medicationReference: params?.medicationReference ?? medicationReference,packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (1)
192-206: Typo makes the code harder to scanThe variable
resouinputResourcesces(lines 193 & 200) is a copy-paste artifact and hampers readability.- const resouinputResourcesces = [ + const inputResources = [ makeCondition({ recorder: makeReference(missingFirstLevel) }, patientId), ]; ... - resources: resouinputResourcesces, + resources: inputResources, ... - expect(res).toEqual(resouinputResourcesces); + expect(res).toEqual(inputResources);packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (2)
155-197: Consider refactoring the nested conditionals for better maintainability.While the selective hydration logic is functionally correct, the nested structure with repetitive patterns could be improved for better readability and maintainability.
Consider extracting the hydration logic into a more declarative configuration:
- const resourcesByType = groupBy(resources, "resourceType"); - Object.entries(resourcesByType).forEach(([currentResourceType, resourcesOfCurrentType]) => { - const { missingReferences } = getReferencesFromResources({ - resourcesToCheckRefs: resourcesOfCurrentType, - sourceResources: resources, - }); - - if (missingReferences.length < 1) return; - - const missingRefsByType = groupBy(missingReferences, "type"); - - const missingPractitioners = get(missingRefsByType, "Practitioner"); - if (missingPractitioners && missingPractitioners.length > 0) { - resourcesToHydrate.push(...missingPractitioners); - } - - const missingOrgs = get(missingRefsByType, "Organization"); - if (missingOrgs && missingOrgs.length > 0) { - resourcesToHydrate.push(...missingOrgs); - } - - if (currentResourceType === "DiagnosticReport") { - const missingEncounters = get(missingRefsByType, "Encounter"); - if (missingEncounters && missingEncounters.length > 0) { - resourcesToHydrate.push(...missingEncounters); - } - const missingObservations = get(missingRefsByType, "Observation"); - if (missingObservations && missingObservations.length > 0) { - resourcesToHydrate.push(...missingObservations); - } - } - - if (medRelatedResourceTypes.includes(currentResourceType as ResourceType)) { - medRelatedResourceTypes.forEach(medRelatedResourceType => { - const missingMedRelatedResources = get(missingRefsByType, medRelatedResourceType); - if (missingMedRelatedResources && missingMedRelatedResources.length > 0) { - resourcesToHydrate.push(...missingMedRelatedResources); - } - }); - } - }); + // Define hydration rules by resource type + const hydrationRules: Record<ResourceType, ResourceType[]> = { + // All resources can hydrate these common types + "*": ["Practitioner", "Organization"], + // Specialized hydration rules + DiagnosticReport: ["Encounter", "Observation"], + ...medRelatedResourceTypes.reduce((acc, type) => ({ + ...acc, + [type]: medRelatedResourceTypes, + }), {}), + }; + + const resourcesByType = groupBy(resources, "resourceType"); + Object.entries(resourcesByType).forEach(([currentResourceType, resourcesOfCurrentType]) => { + const { missingReferences } = getReferencesFromResources({ + resourcesToCheckRefs: resourcesOfCurrentType, + sourceResources: resources, + }); + + if (missingReferences.length < 1) return; + + const missingRefsByType = groupBy(missingReferences, "type"); + + // Apply common hydration rules + hydrationRules["*"]?.forEach(refType => { + const missingRefs = missingRefsByType[refType]; + if (missingRefs?.length > 0) { + resourcesToHydrate.push(...missingRefs); + } + }); + + // Apply specialized hydration rules + const specializedRules = hydrationRules[currentResourceType as ResourceType]; + specializedRules?.forEach(refType => { + const missingRefs = missingRefsByType[refType]; + if (missingRefs?.length > 0) { + resourcesToHydrate.push(...missingRefs); + } + }); + });
232-237: Consider removing the unnecessary generic getter function.The
getfunction doesn't provide additional value over TypeScript's built-in type safety for object property access.-function get<K extends ResourceType, T extends ReferenceWithIdAndType<Resource>[]>( - obj: Dictionary<T>, - key: K -): T | undefined { - return obj[key]; -}Then update the usage to direct property access:
- const missingPractitioners = get(missingRefsByType, "Practitioner"); + const missingPractitioners = missingRefsByType["Practitioner"];packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts (1)
149-158: Simplify the makeEncounter function implementation.The function mutates the encounter object after creation and has redundant initialization logic.
export function makeEncounter( params: Partial<Encounter> | undefined, patientId: string ): MarkRequired<Encounter, "id"> { - const encounter = makeEncounterImported(params, { patient: patientId }); - if (!params?.participant) encounter.participant = []; - if (!params?.location) encounter.location = []; - if (!encounter.id) throw new Error("Encounter ID is required"); - return { ...encounter, id: encounter.id }; + const encounter = makeEncounterImported( + { + participant: [], + location: [], + ...params, + }, + { patient: patientId } + ); + if (!encounter.id) throw new Error("Encounter ID is required"); + return encounter as MarkRequired<Encounter, "id">; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/api/src/external/fhir/patient/__tests__/count-resources-on-s3.test.ts(1 hunks)packages/core/src/command/ai-brief/filter.ts(2 hunks)packages/core/src/command/consolidated/consolidated-filter.ts(2 hunks)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts(5 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(4 hunks)packages/core/src/external/fhir/__tests__/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/__tests__/medication-reference.ts(1 hunks)packages/core/src/external/fhir/consolidated/consolidated.ts(1 hunks)packages/core/src/external/fhir/shared/bundle.ts(1 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-diagnostic-report.ts(1 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-observation.ts(1 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-organization.ts(1 hunks)packages/utils/src/consolidated/refs-from-consolidated.ts(1 hunks)packages/utils/src/fhir/fhir-converter/count-bundle-refs.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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...
**/*.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
packages/utils/src/consolidated/refs-from-consolidated.tspackages/core/src/command/ai-brief/filter.tspackages/utils/src/fhir/fhir-converter/count-bundle-refs.tspackages/core/src/external/fhir/__tests__/medication-reference.tspackages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-organization.tspackages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-observation.tspackages/core/src/external/fhir/consolidated/consolidated.tspackages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-diagnostic-report.tspackages/core/src/command/consolidated/consolidated-filter.tspackages/core/src/external/fhir/__tests__/medication-dispense.tspackages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.tspackages/core/src/command/consolidated/search/fhir-resource/search-consolidated.tspackages/core/src/external/fhir/shared/bundle.tspackages/api/src/external/fhir/patient/__tests__/count-resources-on-s3.test.tspackages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts
🧠 Learnings (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:82-86
Timestamp: 2025-05-31T21:58:28.480Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts, the mutable array operations using push() on resourcesMutable and hydratedMutable (lines 82-86 and 100-103) have been explicitly accepted as exceptions to the immutability guidelines after previous discussion.
🧬 Code Graph Analysis (5)
packages/core/src/command/consolidated/consolidated-filter.ts (1)
packages/core/src/external/fhir/shared/bundle.ts (1)
getReferencesFromResources(73-98)
packages/core/src/external/fhir/__tests__/medication-dispense.ts (1)
packages/core/src/external/fhir/__tests__/medication-reference.ts (1)
makeMedicationReference(3-5)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts (7)
packages/core/src/domain/__tests__/patient.ts (1)
makePatient(56-66)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.ts (1)
makeCondition(6-31)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-encounter.ts (1)
makePractitioner(5-11)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-organization.ts (1)
makeOrganization(5-15)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-observation.ts (1)
makeObservation(5-16)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-diagnostic-report.ts (1)
makeDiagnosticReport(5-24)packages/core/src/external/fhir/__tests__/medication-dispense.ts (1)
makeMedicationDispense(5-22)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (3)
packages/core/src/external/fhir/shared/bundle.ts (2)
ReferenceWithIdAndType(54-55)getReferencesFromResources(73-98)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts (2)
patientId(30-30)cxId(28-28)packages/core/src/external/opensearch/shared/id.ts (1)
getEntryId(9-11)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (5)
packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.ts (1)
makeCondition(6-31)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts (7)
patientId(30-30)genericHydration(40-63)specializedHydration(91-147)Entry(33-38)patient(29-29)makePractitionerWithOrg(160-168)nonSpecializedHydration(65-89)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
hydrateMissingReferences(140-230)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-organization.ts (1)
makeOrganization(5-15)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-encounter.ts (1)
makePractitioner(5-11)
🔇 Additional comments (20)
packages/api/src/external/fhir/patient/__tests__/count-resources-on-s3.test.ts (1)
131-131: LGTM! Typo correction.Good catch fixing the typo from "AllergyIntollerances" to "AllergyIntolerances" in the error message.
packages/utils/src/consolidated/refs-from-consolidated.ts (1)
44-44: LGTM! Parameter renaming for clarity.The parameter name change from
resourcestoresourcesToCheckRefsimproves clarity and aligns with the updated function signature in the shared bundle utilities.packages/utils/src/fhir/fhir-converter/count-bundle-refs.ts (1)
95-95: LGTM! Consistent parameter renaming.The parameter name change to
resourcesToCheckRefsmaintains consistency with the broader refactoring effort across the codebase.packages/core/src/command/ai-brief/filter.ts (2)
1-8: LGTM! Improved type imports.Adding the explicit
ResourceTypeimport improves type safety and supports the type annotations added below.
36-38: LGTM! Explicit type annotations.Adding explicit
ResourceType[]type annotations for thereferenceResourcesandrelevantResourcesarrays improves type safety and code clarity, following TypeScript best practices.packages/core/src/external/fhir/consolidated/consolidated.ts (1)
99-99: LGTM! Con 8000 sistent parameter renaming.The parameter name change to
resourcesToCheckRefsin the hydration loop maintains consistency with the coordinated refactoring effort and improves code clarity.packages/core/src/command/consolidated/consolidated-filter.ts (2)
116-118: LGTM! Parameter renaming aligns with updated function signature.The parameter name change from
resourcestoresourcesToCheckRefscorrectly aligns with the updatedgetReferencesFromResourcesfunction signature, improving code clarity and consistency.
129-131: LGTM! Consistent parameter renaming maintained.The second call site is correctly updated with the same parameter name change, maintaining consistency across the function.
packages/core/src/external/fhir/__tests__/medication-reference.ts (1)
1-5: Well-implemented test utility function.The function follows coding guidelines correctly:
- Uses camelCase for function and parameter names
- Implements functional programming style (small, deterministic, single purpose)
- Properly uses nullish coalesce operator (
??)- Meaningful naming that clearly indicates its purpose
- Consistent with other test utility patterns in the codebase
packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-organization.ts (2)
5-8: LGTM! Well-implemented optional parameter addition.The optional
patientIdparameter is correctly typed and positioned, maintaining backward compatibility while enabling more specific test data generation.
11-11: LGTM! Proper parameter passing to makeSubjectReference.The
patientIdparameter is correctly passed tomakeSubjectReference, enabling context-aware subject reference generation.packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-diagnostic-report.ts (2)
5-8: LGTM! Consistent implementation of optional patientId parameter.The optional
patientIdparameter addition follows the same pattern as other test utility functions, maintaining consistency across the test suite while enabling more specific test data generation.
10-10: LGTM! Correct parameter usage.The
patientIdparameter is properly passed tomakeSubjectReference, aligning with the coordinated updates across test utilities.packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-observation.ts (1)
5-12: 👍 Constructor extension looks goodThe new
patientIdparameter propagates context tomakeSubjectReference, enabling patient-scoped observations.
No functional or style issues spotted; call-sites just need to be updated to pass the extra argument (or rely on the new default).packages/core/src/external/fhir/shared/bundle.ts (1)
84-87:⚠️ Potential issueBug:
flatMapsplits UUIDs into characters — breaks missing-reference detection
Array.prototype.flatMapflattens iterables, so when the callback returns a string the string is treated as an array of chars:["abc"].flatMap(x => x) // -> ['a','b','c']Consequences:
•sourceResourceIdsbecomes a set of single characters,
•missingReferencesis always the full list (nothing matches),
• Hydration will fetch far more resources than required.Fix:
- const sourceResourceIds = new Set(sourceResources.flatMap(r => r.id ?? [])); + const sourceResourceIds = new Set( + sourceResources + .map(r => r.id) + .filter((id): id is string => typeof id === "string") + );(similar change may be needed anywhere else you used
flatMap+ string)⛔ Skipped due to learnings
Learnt from: leite08 PR: metriport/metriport#3797 File: packages/core/src/external/opensearch/text-ingestor-direct.ts:180-182 Timestamp: 2025-05-07T15:08:04.478Z Learning: In JavaScript/TypeScript, when `flatMap()` encounters a string returned by the mapping function, it treats the string as a single element in the output array, not as an array of characters to be flattened.packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (1)
254-260: Circular-reference test only confirms first fetchThe expectation
getByIds_mock“called Times(1)” assumes second-level hydration never happens, but the setup queues a secondmockResolvedValueOnce. Either:
- Remove the second
mockResolvedValueOnce, or- Expect 2 calls and assert that the second set of IDs matches
org1.This will keep the test meaningful even after internal changes.
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (4)
1-1: LGTM!The addition of
ResourceTypeimport is necessary for the new type-specific hydration logic.
27-32: Good constant definition following coding guidelines.The
medRelatedResourceTypesarray properly groups medication-related resource types and follows the naming conventions.
153-153: Important guard condition added to prevent infinite recursion.This iteration guard effectively prevents potential infinite loops during hydration, which is crucial for system stability.
199-204: LGTM! Clean implementation of ID extraction.The logic correctly filters out patient IDs and transforms references into entry IDs for OpenSearch lookup.
...es/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts
Outdated
Show resolved
Hide resolved
...es/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (1)
8-9: Verify import source consistency.The import of
makePractitionerfrom the encounter test file seems inconsistent with its usage context. Consider importing it from a more appropriate shared utility module or the new setup module.-import { makePractitioner } from "../../../../../fhir-to-cda/cda-templates/components/__tests__/make-encounter"; +import { makePractitioner } from "./search-consolidated-setup";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/command/consolidated/search/fhir-resource/tests/search-consolidated-setup.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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...
**/*.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
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts
⏰ 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)
🔇 Additional comments (6)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (6)
49-84: Excellent parameterized test refactoring!The parameterized test approach significantly improves maintainability and reduces code duplication. The generic function with proper typing is well-implemented and follows functional programming principles.
119-152: Good test organization for non-specialized hydration.The parameterized approach is consistently applied here, maintaining good separation of concerns and test clarity.
154-189: Consistent parameterized test pattern for specialized hydration.The test structure maintains consistency with the other parameterized test groups, promoting code reusability and maintainability.
252-270: Good refactor to organization-based circular references.The change from encounter-location-practitioner chains to organization partOf cycles is more realistic and better tests the circular reference handling logic. The test structure is clear and well-organized.
321-360: Well-structured utility functions with proper typing.The
makeToEntryIdandmakeToGetByIdsResultEntryfunctions are well-implemented with proper error handling and TypeScript typing. They follow functional programming principles with clear input/output relationships.
272-317:✅ Verification successful
Verify the expected resource count calculation.
The test expects 3 additional resources (line 296), but the logic suggests it should be 2 practitioners + 1 organization = 3 additional resources, which appears correct. However, please verify this calculation matches the actual hydration behavior.
🏁 Script executed:
#!/bin/bash # Description: Verify the hydration logic for multiple references to the same resource # Expected: Should find references to hydration logic and count calculations # Search for hydration function implementation ast-grep --pattern 'function hydrateMissingReferences($$$) { $$$ }' # Search for any resource counting logic rg -A 10 "hydrateMissingReferences.*resources" --type tsLength of output: 9219
We need to locate the actual hydrateMissingReferences implementation to confirm deduplication and level-by-level fetch behavior.
🏁 Script executed:
#!/usr/bin/env bash # Find where hydrateMissingReferences is defined rg -n "hydrateMissingReferences" --type tsLength of output: 2687
Gathering the implementation of hydrateMissingReferences for inspection…
🏁 Script executed:
#!/usr/bin/env bash sed -n '130,260p' packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.tsLength of output: 3937
Gathering
maxHydrationAttemptsvalue…
🏁 Script executed:
#!/usr/bin/env bash rg -n "maxHydrationAttempts" -n packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.tsLength of output: 211
Expected resource count is correct—no action needed.
The
hydrateMissingReferencesfunction first adds the two missing practitioners (iteration 1) and then the single organization (iteration 2), for a total of three additional resources, matching the test’sresources.length + 3assertion.
...ges/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts
Outdated
Show resolved
Hide resolved
839d18f to
5050c2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
246-251: Consider improving the helper function's type safety and naming.The
getfunction provides type safety for dictionary access, but there are some potential improvements:
Naming: The function name
getis quite generic and could conflict with other utility functions. Consider a more specific name likegetReferencesOfTypeorgetTypedReferences.Return type consistency: The function returns
T | undefinedbut the usage pattern suggests it will always return an array type. Consider constraining the generic type more specifically.-function get<K extends ResourceType, T extends ReferenceWithIdAndType<Resource>[]>( - obj: Dictionary<T>, - key: K -): T | undefined { - return obj[key]; -} +function getReferencesOfType<K extends ResourceType>( + referencesGroupedByType: Dictionary<ReferenceWithIdAndType<Resource>[]>, + resourceType: K +): ReferenceWithIdAndType<Resource>[] | undefined { + return referencesGroupedByType[resourceType]; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/api/src/external/fhir/patient/__tests__/count-resources-on-s3.test.ts(1 hunks)packages/core/src/command/ai-brief/filter.ts(2 hunks)packages/core/src/command/consolidated/consolidated-filter.ts(2 hunks)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts(5 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(6 hunks)packages/core/src/external/fhir/__tests__/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/__tests__/medication-reference.ts(1 hunks)packages/core/src/external/fhir/consolidated/consolidated.ts(1 hunks)packages/core/src/external/fhir/shared/bundle.ts(1 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-diagnostic-report.ts(1 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-observation.ts(1 hunks)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-organization.ts(1 hunks)packages/utils/src/consolidated/refs-from-consolidated.ts(1 hunks)packages/utils/src/fhir/fhir-converter/count-bundle-refs.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/api/src/external/fhir/patient/tests/count-resources-on-s3.test.ts
- packages/utils/src/fhir/fhir-converter/count-bundle-refs.ts
- packages/utils/src/consolidated/refs-from-consolidated.ts
- packages/core/src/command/consolidated/consolidated-filter.ts
- packages/core/src/external/fhir/tests/medication-reference.ts
- packages/core/src/fhir-to-cda/cda-templates/components/tests/make-observation.ts
- packages/core/src/external/fhir/consolidated/consolidated.ts
- packages/core/src/external/fhir/tests/medication-dispense.ts
- packages/core/src/fhir-to-cda/cda-templates/components/tests/make-organization.ts
- packages/core/src/command/ai-brief/filter.ts
- packages/core/src/fhir-to-cda/cda-templates/components/tests/make-diagnostic-report.ts
- packages/core/src/command/consolidated/search/fhir-resource/tests/search-consolidated.test.ts
- packages/core/src/external/fhir/shared/bundle.ts
- packages/core/src/command/consolidated/search/fhir-resource/tests/search-consolidated-setup.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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...
**/*.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
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
🧠 Learnings (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:82-86
Timestamp: 2025-05-31T21:58:28.480Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts, the mutable array operations using push() on resourcesMutable and hydratedMutable (lines 82-86 and 100-103) have been explicitly accepted as exceptions to the immutability guidelines after previous discussion.
🧬 Code Graph Analysis (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (4)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts (3)
patient(29-29)patientId(30-30)cxId(28-28)packages/shared/src/common/date.ts (1)
elapsedTimeFromNow(63-68)packages/core/src/external/fhir/shared/bundle.ts (3)
buildBundleEntry(215-221)ReferenceWithIdAndType(54-55)getReferencesFromResources(73-98)packages/core/src/external/opensearch/shared/id.ts (1)
getEntryId(9-11)
⏰ 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)
🔇 Additional comments (7)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (7)
1-1: LGTM: Imports correctly added for the new functionality.The new imports (
ResourceType,Dictionary,groupBy,ReferenceWithIdAndType) are appropriate for the selective hydration functionality being implemented.Also applies to: 6-6, 13-13
27-32: LGTM: Well-defined medication-related resource types constant.The
medRelatedResourceTypesconstant is correctly defined and follows the naming conventions. It appropriately groups medication-related FHIR resource types for selective hydration.
101-106: LGTM: Function call updated correctly.The function call to
hydrateMissingReferencescorrectly passes the required parameters and the variable naming is consistent with the function's updated signature.
108-114: LGTM: Patient resource handling implemented correctly.The logic correctly filters out Patient resources from the hydrated results and then re-adds the patient resource from the original patient domain object. This ensures Patient resources are not duplicated through the hydration process while maintaining the expected patient resource in the final result.
142-153: LGTM: Documentation updated appropriately.The function documentation clearly explains the new behavior, including the exclusion of Patient resources from hydration and the new
iterationparameter for controlling recursion depth.
213-218: LGTM: Updated variable names and logic flow.The updated variable name
resourcesToHydrateis more descriptive than the previous implementation, and the early return logic correctly handles cases where no resources need hydration.
167-211:✅ Verification successful
Verify the selective hydration logic for correctness and completeness.
The refactored selective hydration logic implements a sophisticated approach by grouping resources by type and selectively hydrating references based on specific resource type combinations. However, there are several areas that need verification:
DiagnosticReport-specific logic: Only DiagnosticReport resources hydrate Encounter and Observation references. Verify this is the intended behavior and not too restrictive.
Medication-related hydration: The logic hydrates all medication-related references when the current resource is medication-related. This could potentially lead to extensive hydration chains.
Missing resource types: Consider if other resource types (like Location, Device, etc.) should also be included in selective hydration scenarios.
🏁 Script executed:
#!/bin/bash # Description: Verify the usage patterns of different resource types in FHIR references to validate the selective hydration logic. echo "=== Checking DiagnosticReport references to Encounter and Observation ===" ast-grep --pattern $'DiagnosticReport { $$$ encounter: $$$ $$$ }' echo "=== Checking if other resource types reference Encounter/Observation ===" rg -A 3 '"encounter":|"observation":' --type ts echo "=== Checking medication resource reference patterns ===" rg -A 5 'medication.*reference|reference.*medication' --type tsLength of output: 33522
Selective hydration logic confirmed
A review of the codebase shows that:
- Only DiagnosticReport resources actually reference Encounter and Observation fields and are explicitly hydrated for those types.
- All medication-related resources are scoped to medRelatedResourceTypes, and there’s no indication of unintended or cascading hydration beyond what’s defined.
- There are currently no Location or Device reference patterns in the code, so no additional types need to be added today.
No changes required. Revisit this logic if new resource types (e.g. Location, Device) are introduced.
a4c553f to
2758195
Compare
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
5050c2a to
d29e1fd
Compare
...es/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts
Outdated
Show resolved
Hide resolved
...ges/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts
Show resolved
Hide resolved
...ges/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts
Show resolved
Hide resolved
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
Outdated
Show resolved
Hide resolved
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/external/opensearch/lexical/fhir-searcher.ts (1)
44-55: Consider adding defensive error handling around the OpenSearch call
paginatedSearchmay throw (e.g., network error, malformed query). Wrapping the request in atry / catch—similar to whatgetByIdalready does—will prevent an unhandled rejection from bubbling up and give you a single, consistent place to log/capture the failure.- const response = await paginatedSearch<FhirSearchResult>({ + let response; + try { + response = await paginatedSearch<FhirSearchResult>({ client, indexName, searchRequest: searchQuery, mapResults, - }); + }); + } catch (error) { + const msg = "Error searching OpenSearch"; + log(`${msg}: ${errorToString(error)}`); + capture.error(msg, { extra: { context: this.constructor.name, error: errorToString(error) } }); + throw error; + }packages/core/src/external/opensearch/lexical/query.ts (1)
27-28: Drop the superfluous ternary
isReturnAllResourcescan be assigned more directly, improving readability and satisfying the static-analysis warning.- const isReturnAllResources = actualQuery && actualQuery.length > 0 ? false : true; + const isReturnAllResources = !(actualQuery && actualQuery.length > 0);🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (2)
190-199: Risk of oversize / malformed simple-query-string when injecting many IDs
query = \${simpleQueryStringPrefix} "${uniqueIds.join('" "')}"``
• Thousands of IDs will blow past OpenSearch’s default 32 kB query limit.
• IDs containing quotes/whitespace will corrupt the query.Consider switching to an
idsfilter (preferred) or chunking IDs to stay below the limit.
331-336: Rename helper to avoid shadowing well-knownlodash/getA local util named
getmakes the reader think it’s lodash. Renaming clarifies intent and prevents accidental import clashes.-function get<K extends ResourceType, T extends ReferenceWithIdAndType<Resource>[]>( +function getFromDict<K extends ResourceType, T extends ReferenceWithIdAndType<Resource>[]>( obj: Dictionary<T>, key: K ): T | undefined { return obj[key]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(6 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/opensearch/fhir-ingestor.ts(2 hunks)packages/core/src/external/opensearch/index-based-on-fhir.ts(1 hunks)packages/core/src/external/opensearch/lexical/fhir-searcher.ts(2 hunks)packages/core/src/external/opensearch/lexical/index-create.sh(3 hunks)packages/core/src/external/opensearch/lexical/query.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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...
**/*.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
packages/core/src/external/opensearch/index-based-on-fhir.tspackages/core/src/external/fhir/export/string/shared/reference.tspackages/core/src/external/opensearch/fhir-ingestor.tspackages/core/src/external/opensearch/lexical/query.tspackages/core/src/external/opensearch/lexical/fhir-searcher.tspackages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts
🧠 Learnings (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (2)
Learnt from: leite08
PR: metriport/metriport#3940
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:82-86
Timestamp: 2025-05-31T21:58:28.502Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts, the mutable array operations using push() on resourcesMutable and hydratedMutable (lines 82-86 and 100-103) have been explicitly accepted as exceptions to the immutability guidelines after previous discussion.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
🪛 Biome (1.9.4)
packages/core/src/external/opensearch/lexical/query.ts
[error] 27-27: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (6)
packages/core/src/external/opensearch/index-based-on-fhir.ts (1)
10-10: LGTM - Good incremental migration approach.Adding the
ingestionDatefield as optional with a clear TODO for making it required after migration is a solid approach for production systems. This supports the consolidated search improvements while maintaining backward compatibility.packages/core/src/external/opensearch/fhir-ingestor.ts (2)
3-3: LGTM - Correct date utility import.Following the coding guidelines by using
buildDayjsfrom shared utilities instead of direct dayjs usage.
269-269: LGTM - Proper ingestion timestamp implementation.Correctly sets the ingestion date to current ISO timestamp, which aligns with the optional field added to
FhirIndexFieldstype and supports the consolidated search improvements.packages/core/src/external/opensearch/lexical/index-create.sh (2)
5-5: LGTM - Proper index versioning for breaking change.The index name bump from "consolidated-data-1" to "consolidated-data-2" with alias update correctly handles the schema change for adding
ingestionDate. Theis_write_index: truedesignation properly configures the alias for write operations.Ensure the index migration is coordinated with deployment to avoid data loss or downtime.
Also applies to: 46-48
27-29: LGTM - Correct field mapping for ingestion date.The
ingestionDatefield mapping with type "date" correctly supports the ISO timestamp format being set in the application code.packages/core/src/external/fhir/export/string/shared/reference.ts (1)
37-40:❓ Verification inconclusive
LGTM - Improved reference formatting logic.
The new implementation is more robust by handling both
referenceanddisplayfields, trimming whitespace, and filtering falsy values. This provides better reference representation for the consolidated search functionality.Verify that the change in output format (joining reference and display with space) doesn't break existing consumers of this function:
🏁 Script executed:
#!/bin/bash # Description: Find usages of formatReference to assess impact of format change ast-grep --pattern 'formatReference($_)'Length of output: 16880
🏁 Script executed:
#!/bin/bash # Inspect the formatReference function signature and implementation context rg -n "formatReference" -A5 packages/core/src/external/fhir/export/string/shared/reference.tsLength of output: 648
Verify downstream handling of the new space-separated format
The updated
formatReferencenow builds its return value by trimming and joiningreference.referenceandreference.displaywith a single space, returningundefinedonly if both are empty, and prefixing with"Label: "whenisDebugistrue. While this is more robust, it does change the literal output string.I ran a search and found the definition plus ~70 call sites under:
packages/core/src/external/fhir/export/string/shared/reference.ts(implementation)packages/core/src/external/fhir/export/string/resources/*(all resource files, e.g.,document-reference.ts,procedure.ts,coverage.ts, etc.)Please review these consumers to ensure:
- No downstream formatting or parsing logic assumes the old delimiter or output shape.
- Tests and documentation are updated to reflect
"id display"(and"Label: id display") instead of the previous format.
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts(1 hunks)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts(6 hunks)packages/core/src/external/opensearch/lexical/query.ts(1 hunks)packages/utils/src/consolidated/refs-from-consolidated.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/utils/src/consolidated/refs-from-consolidated.ts
- packages/core/src/command/consolidated/search/fhir-resource/tests/search-consolidated.test.ts
- packages/core/src/command/consolidated/search/fhir-resource/tests/search-consolidated-setup.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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...
**/*.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
packages/core/src/external/opensearch/lexical/query.ts
🧬 Code Graph Analysis (1)
packages/core/src/external/opensearch/lexical/query.ts (3)
packages/core/src/external/opensearch/index.ts (2)
OpenSearchRequestBody(36-36)contentFieldName(1-1)packages/core/src/external/opensearch/shared/query.ts (4)
simpleQueryStringPrefix(3-3)cleanupQuery(53-56)getGeneralParams(33-41)getPatientFilters(43-45)packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts (2)
cxId(28-28)patientId(30-30)
🪛 Biome (1.9.4)
packages/core/src/external/opensearch/lexical/query.ts
[error] 27-27: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (2)
packages/core/src/external/opensearch/lexical/query.ts (2)
20-23: LGTM! Function signature correctly returns structured result.The updated return type properly separates the
isReturnAllResourcesflag from thesearchQuery, making the interface cleaner and more explicit.
29-79: LGTM! Fixed generalParams placement and implemented proper conditional logic.The
generalParamsare now correctly spread insidesearchQueryrather than at the top level, addressing the previous review feedback. The conditional logic usingisReturnAllResourcesto include/exclude search clauses is well-structured and maintains the same OpenSearch query functionality while providing cleaner separation of concerns.
Ref eng-403 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Dependencies
Description
Selectively load refs for consolidated search
Testing
Release Plan
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Style