ENG-371 Improve search hydration + Cleaner code #3945
Conversation
WalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SearchModule
participant Hydration
participant DB
Caller->>SearchModule: searchPatientConsolidated(params)
SearchModule->>Hydration: hydrateMissingReferences({..., hydratePatient})
Hydration->>DB: getByIds([missing references])
DB-->>Hydration: [hydrated resources]
Hydration-->>SearchModule: [hydrated resources]
SearchModule->>SearchModule: Filter Patient, convert, append Patient
SearchModule-->>Caller: Bundle with hydrated resources and Patient
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
93-104: Consider keeping the mutable naming convention.The variable was renamed from
hydratedMutabletohydrated, but according to the retrieved learnings, mutable array operations were explicitly accepted in this file. Consider keeping the "Mutable" suffix for consistency with the existingresourcesMutablevariable, making it clear that this array will be mutated.
🧹 Nitpick comments (1)
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
134-145: Documentation is clear but could be more explicit.The JSDoc comment states "Doesn't hydrate the Patient resource in the resources array" but this is only true when
hydratePatientis false (the default). Consider updating the comment to clarify this conditional behavior.Apply this diff to clarify the documentation:
/** * Hydrates the missing references in the resources. * - * Doesn't hydrate the Patient resource in the resources array. + * By default, doesn't hydrate the Patient resource unless hydratePatient is set to true. * * @param cxId - The customer ID. * @param patientId - The patient ID. * @param resources - The resources to hydrate. * @param iteration - The iteration of the hydration. * @param hydratePatient - Whether to hydrate the Patient resource. Optional, defaults to false. * @returns The hydrated resources. */
📜 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.test.ts(3 hunks)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts(3 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/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.tspackages/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/__tests__/search-consolidated.test.ts (4)
packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-encounter.ts (1)
makeEncounter(37-57)packages/core/src/fhir-to-cda/cda-templates/components/__tests__/make-condition.ts (1)
makeCondition(6-31)packages/api/src/command/medical/patient/__tests__/store-query-cmd.ts (1)
patient(15-15)packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (1)
hydrateMissingReferences(146-196)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated.test.ts (3)
127-127: LGTM! Good boundary testing.Starting the iteration at 6 (beyond the max of 5) properly tests the edge case of exceeding maximum hydration attempts.
152-152: LGTM! Clear test name update.The test name now accurately reflects that ignoring patient references is the default behavior.
176-203: Well-structured test for the newhydratePatientparameter.The test comprehensively verifies:
- Patient resource is included in hydration when
hydratePatient: true- The hydrated results contain all expected resources
- The
getByIdscall includes both encounter and patient IDsGood coverage of the new functionality.
packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts (4)
146-158: LGTM! Clean parameter addition.The new
hydratePatientparameter with a default value offalsemaintains backward compatibility while enabling the new functionality.
161-161: LGTM! Proper guard clause placement.Moving the iteration check to the beginning of the function is a cleaner approach than checking it later in the logic flow.
166-166: LGTM! Correct conditional logic.The logic properly filters out the patient ID only when
hydratePatientis false, enabling explicit control over patient hydration.
100-104: Good separation of concerns for patient handling.The approach of filtering out patient resources and then explicitly adding the patient back ensures no duplicates and clear control over the patient resource in the results.
304e2e7 to
a4c553f
Compare
|
|
||
| const patientResource = patientToFhir(patient); | ||
| hydratedMutable.push(patientResource); | ||
| const withPatient = [...withoutPatient, patientResource]; |
| const withoutPatient = hydrated.filter(r => r.resourceType !== "Patient"); | ||
|
|
||
| const patientResource = patientToFhir(patient); | ||
| hydratedMutable.push(patientResource); | ||
| const withPatient = [...withoutPatient, patientResource]; |
There was a problem hiding this comment.
@leite08 whats the context for filtering all patients out and then adding the patient back in? Avoids dupes?
There was a problem hiding this comment.
So we use the pt from the DB, the source of truth - we don't update consolidated when we update the pt
| return getEntryId(cxId, patientId, referenceId); | ||
| }); | ||
| if (missingRefIds.length < 1 || iteration >= maxHydrationAttempts) return resources; | ||
| if (missingRefIds.length < 1) return resources; |
There was a problem hiding this comment.
hypernit; imo prefer === 0 over < 1 when possible.
There was a problem hiding this comment.
Defensive programming, code in a way that even w/ smaller bugs the code keeps working. I don't expect this from array.length, but it's a programming style... :)
There was a problem hiding this comment.
haha ok makes sense i guess
- search hydration w/ right amount of iterations - clean up the code that inserts the patient in the search result Ref eng-371 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
a4c553f to
2758195
Compare
Dependencies
Description
Testing
Release Plan
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests