ENG-268 Review FHIR resources to string #3869
Conversation
WalkthroughThis change significantly enhances the FHIR resource-to-string conversion framework. It introduces string conversion classes for multiple new FHIR resource types, expands and modularizes formatting utilities, and enforces minimum data checks for string output. A new utility function enables conversion of single FHIR resources to strings. Numerous helper functions for formatting FHIR elements are added or refactored. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant resourceToString
participant ResourceToStringClass
participant FormatterUtils
Client->>resourceToString: Call with FHIR resource
resourceToString->>ResourceToStringClass: Select converter by resource type
ResourceToStringClass->>FormatterUtils: Format fields (e.g., identifiers, names, codes)
FormatterUtils-->>ResourceToStringClass: Return formatted strings
ResourceToStringClass-->>resourceToString: Return concatenated string or undefined
resourceToString-->>Client: Return string or undefined
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 warn config production Use ✨ 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: 20
🔭 Outside diff range comments (4)
packages/core/src/external/fhir/export/string/resources/procedure.ts (2)
13-81: 🛠️ Refactor suggestionMissing minimum data check pattern.
Unlike ImmunizationToString, this class doesn't implement the minimum data check pattern using hasMinimumData flag. This inconsistency could lead to empty strings being returned instead of undefined when no meaningful data is present.
toString(procedure: Procedure): string | undefined { const parts: string[] = []; + let hasMinimumData = false; const identifierStr = formatIdentifiers(procedure.identifier); if (identifierStr) { parts.push(identifierStr); + hasMinimumData = true; } if (procedure.status) { parts.push(`Status: ${procedure.status}`); } if (procedure.category) { const categoryStr = formatCodeableConcept(procedure.category); if (categoryStr) { parts.push(`Category: ${categoryStr}`); } } if (procedure.code) { const codeStr = formatCodeableConcept(procedure.code); if (codeStr) { parts.push(`Code: ${codeStr}`); + hasMinimumData = true; } } // ... [rest of the code] + if (!hasMinimumData) return undefined; return parts.join(FIELD_SEPARATOR); }
1-84: 💡 Verification agent🧩 Analysis chain
Consider adding formatIdentifiers implementation.
The formatIdentifiers function (imported and used on line 16) is a stub that always returns undefined based on the relevant code snippet. Consider implementing this function properly or handling the undefined return value explicitly.
🏁 Script executed:
#!/bin/bash # Check if formatIdentifiers is implemented in any other file grep -r "export function formatIdentifiers" --include="*.ts" packages/core/src/external/fhir/export/string/Length of output: 271
To verify whether
formatIdentifiersis indeed just a stub, let’s inspect its implementation:
🏁 Script executed:
#!/bin/bash sed -n '1,200p' packages/core/src/external/fhir/export/string/shared/identifier.tsLength of output: 487
Implement identifier formatting in shared/identifier.ts
The
formatIdentifiers(and its siblingformatIdentifier) in
packages/core/src/external/fhir/export/string/shared/identifier.ts
are currently stubs that always returnundefined. As a result, any identifiers on yourProcedureresource will never be rendered byProcedureToString.Please update these functions to serialize FHIR Identifier objects—for example, concatenating each identifier’s
systemandvalue(and other relevant fields) with theFIELD_SEPARATOR, similar to how other formatters are implemented.• File needing changes:
- packages/core/src/external/fhir/export/string/shared/identifier.ts (lines 1–11)
• Impacted usage:
- ProcedureToString.toString (packages/core/src/external/fhir/export/string/resources/procedure.ts)
Once implemented, identifiers will correctly appear when you call
formatIdentifiers(...). Add or update tests to cover identifier formatting as well.packages/core/src/external/fhir/export/string/resources/medication.ts (1)
13-87: 🛠️ Refactor suggestionMissing minimum data check
Unlike other implementations, such as LocationToString, this class doesn't implement minimum data validation before returning a string.
Consider adding a minimum data check to align with the pattern used in other resource converters:
toString(medication: Medication): string | undefined { + let hasMinimumData = defaultHasMinimumData; const parts: string[] = []; // ...existing code... if (medication.code) { const codeStr = formatCodeableConcepts([medication.code], "Code"); if (codeStr) { parts.push(codeStr); + hasMinimumData = true; } } // ...rest of existing code... + if (!hasMinimumData) return undefined; return parts.join(FIELD_SEPARATOR); }packages/core/src/external/fhir/export/string/resources/practitioner.ts (1)
13-54: 🛠️ Refactor suggestionMissing minimum data check
Unlike the LocationToString implementation, this class doesn't check for minimum data before returning a string. If all formatting functions return undefined, it could return an empty string instead of undefined.
Consider adding a minimum data check:
toString(practitioner: Practitioner): string | undefined { + let hasMinimumData = defaultHasMinimumData; const parts: string[] = []; const identifierStr = formatIdentifiers(practitioner.identifier); if (identifierStr) parts.push(identifierStr); const names = formatHumanNames(practitioner.name); if (names) { parts.push(names); + hasMinimumData = true; } // ... rest of existing code ... + if (!hasMinimumData) return undefined; return parts.join(FIELD_SEPARATOR); }Also, don't forget to add the import:
import { Practitioner } from "@medplum/fhirtypes"; -import { FHIRResourceToString } from "../fhir-resource-to-string"; +import { defaultHasMinimumData, FHIRResourceToString } from "../fhir-resource-to-string";
🧹 Nitpick comments (30)
packages/core/src/external/fhir/export/string/shared/telecom.ts (1)
21-36: Add JSDoc comment forformatTelecomfunctionThe function
formatTelecomis missing a JSDoc comment describing its purpose and parameters, while theformatTelecomsfunction has one. For consistency and better code documentation, add a JSDoc comment similar to the one forformatTelecoms.+/** + * Formats a single FHIR telecom contact point into a string representation + * @param telecom - FHIR contact point to format + * @param label - Optional label to prefix the formatted string + * @returns Formatted string of telecom or undefined if no data + */ export function formatTelecom( telecom: ContactPoint | undefined, label?: string ): string | undefined {packages/core/src/external/fhir/export/string/shared/age.ts (1)
8-10: Consider using regular function declaration instead of arrow functionAccording to the coding guidelines, arrow functions should be avoided. While this is a simple wrapper, consider using a regular function declaration for consistency with other utility functions.
-export const formatAge = (age: Age | undefined, label?: string): string | undefined => { - return formatQuantity(age, label); -}; +export function formatAge(age: Age | undefined, label?: string): string | undefined { + return formatQuantity(age, label); +}packages/core/src/external/fhir/export/string/shared/code-display.ts (1)
1-9: Function looks good but needs JSDoc documentationThis utility function provides a concise way to format code and display strings with appropriate handling of missing values. However, it's missing JSDoc documentation that would help explain its purpose and parameters, especially since other similar functions in the codebase have documentation.
+/** + * Formats code and display strings together into a single string representation + * + * @param code - The code value to format + * @param display - The display value to format + * @returns Formatted string combining code and display, or undefined if both are missing + */ export function codeAndDisplayToString( code: string | undefined, display: string | undefinedpackages/core/src/external/fhir/export/string/shared/duration.ts (2)
4-6: JSDoc comment is incompleteThe JSDoc comment is missing @param and @returns descriptions, which would help document the function's parameters and return value.
/** * Formats a FHIR Duration object into a string representation + * + * @param duration - FHIR Duration object to format + * @param label - Optional label to prefix the formatted string with + * @returns Formatted string representation of the duration, or undefined if the duration is missing or has no content */
7-21: Consider using function declaration instead of arrow functionAccording to the coding guidelines, arrow functions should be avoided. Consider using a function declaration instead.
-export const formatDuration = ( - duration: Duration | undefined, - label?: string -): string | undefined => { +export function formatDuration( + duration: Duration | undefined, + label?: string +): string | undefined {Otherwise, the implementation is well-structured, properly handles missing data, and follows a consistent pattern with other formatting utilities.
packages/core/src/external/fhir/export/string/shared/narrative.ts (4)
6-12: JSDoc comment is incompleteThe JSDoc comment is missing @param and @returns descriptions, which would help document the function's parameters and return value.
/** * Formats a FHIR Narrative into a string representation + * + * @param narrative - FHIR Narrative object to format + * @param label - Label to use in the formatted output + * @returns Formatted string representation of the narrative, or undefined if narrative is missing or has no content */
14-14: Redundant optional chaining operatorThere's a redundant use of the optional chaining operator (
?.) onnarrative.divsince you already check for its existence in the line above.- const filteredDiv = narrative.div?.replace(new RegExp(filterFromDiv.join("|"), "g"), ""); + const filteredDiv = narrative.div.replace(new RegExp(filterFromDiv.join("|"), "g"), "");
11-11: Consider making label parameter optionalUnlike other similar formatting functions in this PR (e.g.,
formatPeriod,formatDuration), thelabelparameter here is required. Consider making it optional for consistency with other formatting utilities.export function formatNarrative( narrative: Narrative | undefined, - label: string + label?: string ): string | undefined { if (!narrative?.div) return undefined; const filteredDiv = narrative.div?.replace(new RegExp(filterFromDiv.join("|"), "g"), ""); if (!filteredDiv || filteredDiv.length < 1) return undefined; const parts: string[] = []; if (narrative.status) { parts.push(`Status: ${narrative.status}`); } - parts.push(`${label}: ${filteredDiv}`); + parts.push(label ? `${label}: ${filteredDiv}` : filteredDiv); return parts.join(FIELD_SEPARATOR); }
14-14: Consider pre-compiling the regular expressionFor better performance, consider pre-compiling the regular expression outside the function, especially if this function is called frequently.
const filterFromDiv = ['<div xmlns="http://www.w3.org/1999/xhtml">', "<div>", "</div>", "<div/>"]; +const filterRegex = new RegExp(filterFromDiv.join("|"), "g"); /** * Formats a FHIR Narrative into a string representation */ export function formatNarrative( narrative: Narrative | undefined, label: string ): string | undefined { if (!narrative?.div) return undefined; - const filteredDiv = narrative.div?.replace(new RegExp(filterFromDiv.join("|"), "g"), ""); + const filteredDiv = narrative.div?.replace(filterRegex, ""); if (!filteredDiv || filteredDiv.length < 1) return undefined;packages/core/src/external/fhir/export/string/resources/patient.ts (1)
12-28: Consider adding minimum data check.Unlike the ImmunizationToString class, PatientToString doesn't implement the minimum data check pattern that returns undefined when no meaningful data is present. This could lead to empty strings being returned.
toString(patient: Patient): string | undefined { const parts: string[] = []; + let hasMinimumData = false; const identifierStr = formatIdentifiers(patient.identifier); - if (identifierStr) parts.push(identifierStr); + if (identifierStr) { + parts.push(identifierStr); + hasMinimumData = true; + } const nameStr = formatHumanNames(patient.name); - if (nameStr) parts.push(nameStr); + if (nameStr) { + parts.push(nameStr); + hasMinimumData = true; + } const addressStr = formatAddresses(patient.address, "Address"); if (addressStr) parts.push(addressStr); if (patient.birthDate) parts.push(`DOB: ${patient.birthDate}`); if (patient.gender) parts.push(`Gender: ${patient.gender}`); + if (!hasMinimumData && parts.length === 0) return undefined; return parts.join(FIELD_SEPARATOR); }packages/core/src/external/fhir/export/string/shared/range.ts (1)
9-9: Consider using function declaration instead of arrow function.The coding guidelines recommend avoiding arrow functions. Consider using a regular function declaration instead.
-export const formatRange = (range: Range | undefined, label?: string): string | undefined => { +export function formatRange(range: Range | undefined, label?: string): string | undefined {packages/core/src/external/fhir/export/string/shared/attachment.ts (1)
10-29: Comprehensive attachment formatting with opportunity for enhancement.The
formatAttachmentfunction handles the core functionality well but has several properties commented out (lines 22-26) that might be useful for searchability.Consider evaluating which of the commented-out properties would enhance searchability and uncommenting those that align with your search requirements. This would improve the comprehensiveness of the string representation.
packages/core/src/external/fhir/export/string/resources/medication-dispense.ts (1)
51-56: Document rationale for excluded fields.The commented-out sections for
subjectanddaysSupplylack explanation for why these fields are excluded from the string representation.Add a brief comment above each commented-out section explaining why these fields are excluded from the search index, similar to what's mentioned in the PR objectives.
+// Subject intentionally excluded from search index // if (dispense.subject) { // const subjectStr = formatReferences([dispense.subject], "Subject"); // if (subjectStr) { // parts.push(subjectStr); // } // }+// Days supply intentionally excluded from search index // if (dispense.daysSupply) { // const daysSupplyStr = formatQuantity(dispense.daysSupply, "Days Supply"); // if (daysSupplyStr) { // parts.push(daysSupplyStr); // } // }Also applies to: 101-106
packages/core/src/external/fhir/export/string/bundle-to-string.ts (2)
53-53: Consider clarifying future plans for Patient resource.The
resourceTypesToSkiparray has been reduced to only include "Patient", which suggests you're expanding coverage to most resources except Patient.Consider adding a comment explaining why Patient is skipped, especially since there's a TODO comment on line 78-79 about removing it.
78-79: Add more context to the TODO comment.This TODO comment lacks specificity about when or why Patient should be removed from the ResourceTypeMap.
Consider adding more context to the TODO, such as "TODO: REMOVE THIS once Patient is supported for conversion" or similar clarification.
packages/core/src/external/fhir/export/string/resources/related-person.ts (1)
34-37: Consider adding more robust null check for person.name.While the current implementation works, consider adding a more explicit null check before accessing person.name to improve code robustness.
- if (person.name) { + if (person.name?.length) { const names = formatHumanNames(person.name); if (names) parts.push(names); }packages/core/src/external/fhir/export/string/resources/consent.ts (2)
80-94: Redundant type annotation in map callback.The explicit type annotation for verification in the map callback is redundant as TypeScript can infer this.
- const verifications = consent.verification - .map((verification: ConsentVerification) => { + const verifications = consent.verification + .map(verification => { const components = [ verification.verified && `Verified`, formatReference(verification.verifiedWith, "Verified By"), verification.verificationDate && `Verification Date: ${verification.verificationDate}`, ].filter(Boolean); return components.join(FIELD_SEPARATOR); }) .filter(Boolean);
96-174: Consider extracting provision formatting to a separate method.The provision formatting logic is quite complex and nested. Consider extracting it to a separate method to improve readability and maintainability of the code.
You could refactor this into a method like:
private formatProvision(provision: ConsentProvision): string | undefined { const provisionParts: string[] = []; if (provision.type) provisionParts.push(`Type: ${provision.type}`); // Add the rest of the formatting logic... return provisionParts.length > 0 ? `Provision: ${provisionParts.join(FIELD_SEPARATOR)}` : undefined; }And then call it from the main method:
if (consent.provision) { const provisionStr = this.formatProvision(consent.provision); if (provisionStr) { parts.push(provisionStr); hasMinimumData = true; } }packages/core/src/external/fhir/export/string/resources/medication-administration.ts (1)
67-81: Consider activating minimum data flag for performer.The
hasMinimumDataflag assignment for performer is commented out, but performer information can be valuable for search context. Consider activating this based on your search requirements.if (performers.length > 0) { parts.push(performers.join(FIELD_SEPARATOR)); - // hasMinimumData = true; + hasMinimumData = true; }packages/core/src/external/fhir/export/string/resources/family-member-history.ts (1)
59-80: Limited deceased information inclusion.The implementation only includes the boolean deceased status while commenting out more detailed information like deceased age, range, date. Consider if additional deceased details would be valuable for search purposes.
Consider including at least the deceased date when available, as this could provide valuable temporal context for searches:
if (history.deceasedBoolean) { parts.push(`Deceased`); } +} else if (history.deceasedDate) { + parts.push(`Deceased Date: ${history.deceasedDate}`); // } else if (history.deceasedAge) { // parts.push(`Deceased Age: ${history.deceasedAge.value} ${history.deceasedAge.unit}`); // } else if (history.deceasedRange) { // const deceasedStr = formatPeriod(history.deceasedRange, "Deceased Range"); // if (deceasedStr) parts.push(deceasedStr); -// } else if (history.deceasedDate) { -// parts.push(`Deceased Date: ${history.deceasedDate}`); // } else if (history.deceasedString) { // parts.push(`Deceased: ${history.deceasedString}`); // }packages/core/src/external/fhir/export/string/resources/observation.ts (1)
38-47: Consider extracting reference range formatting to a helper functionThe reference range formatting logic is complex and might benefit from being extracted to a dedicated helper function in a shared module, similar to other formatting utilities. This would improve reusability and testing.
You could create a new helper function in a shared module:
// In a shared/reference-range.ts file export function formatReferenceRanges( referenceRanges: ObservationReferenceRange[] | undefined, label = "Reference Range" ): string | undefined { if (!referenceRanges?.length) return undefined; const formattedRanges = referenceRanges .map((rr: ObservationReferenceRange) => { const lowStr = formatQuantity(rr.low, "Low"); const highStr = formatQuantity(rr.high, "High"); return [lowStr, highStr].filter(Boolean).join(FIELD_SEPARATOR); }) .filter(Boolean) .join(FIELD_SEPARATOR); return formattedRanges ? `${label}: ${formattedRanges}` : undefined; }Then in this file:
- const referenceRangeStr = observation.referenceRange - ?.map((rr: ObservationReferenceRange) => { - const lowStr = formatQuantity(rr.low, "Low"); - const highStr = formatQuantity(rr.high, "High"); - return [lowStr, highStr].filter(Boolean).join(FIELD_SEPARATOR); - }) - .join(FIELD_SEPARATOR); - if (referenceRangeStr) { - parts.push(`Reference Range: ${referenceRangeStr}`); - } + const referenceRangeStr = formatReferenceRanges(observation.referenceRange); + if (referenceRangeStr) { + parts.push(referenceRangeStr); + }packages/core/src/external/fhir/export/string/shared/address.ts (2)
28-28: Document the intentional exclusion of country fieldThe country field is commented out, which appears intentional but lacks explanation. This aligns with the PR objective that mentions "commented-out code sections that explicitly indicate which properties are excluded from the string conversion process." Consider adding a comment explaining why this field is excluded from the search index.
address.city, address.state, address.postalCode, - // address.country, + // address.country, // Excluded from search index as it's not part of search criteria
32-33: Consider conditionally including address.textThe code adds
address.textunconditionally to the components array, which may duplicate information if text contains the same data as the structured fields. Consider adding a check similar to the one informatCodeableConceptto avoid duplication.- const formatted = [...components, address.text].filter(Boolean).join(FIELD_SEPARATOR); + const text = address.text?.trim(); + const textStr = text && !components.join(FIELD_SEPARATOR).includes(text) ? text : undefined; + const formatted = [...components, textStr].filter(Boolean).join(FIELD_SEPARATOR);packages/core/src/external/fhir/export/string/shared/coding.ts (3)
6-12: Consider makinglabeloptional for broader re-usability
formatCodingscurrently forces callers to pass alabel, yet several existing helpers (e.g.formatDosage,formatCodeableConcept) accept an optional label. Allowinglabel?: stringkeeps the public surface consistent and avoids having to sprinkle empty strings around the codebase.-export function formatCodings(codings: Coding[] | undefined, label: string): string | undefined { +export function formatCodings(codings: Coding[] | undefined, label?: string): string | undefined { … - return label ? `${label}: ${formattedCodings}` : formattedCodings; + return label ? `${label}: ${formattedCodings}` : formattedCodings; }
17-18: Boolean stored in a variable calledsystemis misleading
systemreads as though it holds the system URI, but it actually stores a boolean result fromisAllowedSystem. A clearer name (isSystemAllowed,allowedSystem, …) avoids confusion for future maintainers.- const system = isAllowedSystem(coding.system); - if (!system) return undefined; + const isSystemAllowed = isAllowedSystem(coding.system); + if (!isSystemAllowed) return undefined;
25-27: Passundefinedinstead of an empty string
codeAndDisplayToString("", display)works but relies on an implementation detail that a falsy non-undefined value is ignored. Passingundefinedis semantically correct and avoids accidental double-spaces if the helper ever changes.- const altCode = ""; - const codeAndDisplay = codeAndDisplayToString(altCode, coding.display); + const codeAndDisplay = codeAndDisplayToString(undefined, coding.display);packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
21-22: Optional chaining simplifies nested accessStatic analysis flagged this. Using optional chaining makes the intent clearer and avoids a redundant truthy check.
- dosage.doseAndRate && - dosage.doseAndRate.map(formatDosageDoseAndRate).filter(Boolean).join(FIELD_SEPARATOR), + dosage.doseAndRate?.map(formatDosageDoseAndRate).filter(Boolean).join(FIELD_SEPARATOR),🧰 Tools
🪛 Biome (1.9.4)
[error] 21-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (3)
124-150: Inconsistent use offormatDurationmay duplicate labels
formatDurationis called once without a label (line 124) and later with a label argument (line 145).
Depending on the helper’s signature, this can:
- Emit “Duration: Duration: 30 days” (double prefix) or
- Produce mismatched formatting (one entry labelled, one not).
Recommend standardising the call style—either always supply the label via the helper or always wrap the raw value locally—to ensure predictable output.
-const durationStr = formatDuration(dispense.initialFill.duration); -if (durationStr) initialFillParts.push(`Duration: ${durationStr}`); +const durationStr = formatDuration( + dispense.initialFill.duration, + "Duration" +); +if (durationStr) initialFillParts.push(durationStr);(or the inverse—remove the second parameter at line 145).
Please apply the same convention throughout the file.
58-60: Remove commented-out code or add explanatory rationaleLeaving dead code (
// const subjectStr …) clutters the file and conflicts with the project guideline “only add comments to explain why something was done.”
Either delete these lines or convert them into a proper comment explaining whysubjectis intentionally excluded from the searchable string.
160-175:substitution.allowedBooleanignores an explicitfalseCurrently the string “Substitution Allowed” is appended only when the boolean is true.
FHIR distinguishes between allowed (true) and not allowed (false); omitting thefalsecase loses clinically relevant information.Suggestion:
-if (substitution.allowedBoolean) substitutionParts.push(`Substitution Allowed`); +if (substitution.allowedBoolean === true) { + substitutionParts.push("Substitution Allowed"); +} else if (substitution.allowedBoolean === false) { + substitutionParts.push("Substitution NOT Allowed"); +}This preserves both states while keeping the existing behaviour for
undefined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
packages/core/src/external/fhir/export/string/bundle-to-string.ts(6 hunks)packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/communication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/composition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/condition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/consent.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/coverage.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts(2 hunks)packages/core/src/external/fhir/export/string/resources/document-reference.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/encounter.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/family-member-history.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/immunization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/location.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-administration.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-dispense.ts(3 hunks)packages/core/src/external/fhir/export/string/resources/medication-request.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-statement.ts(2 hunks)packages/core/src/external/fhir/export/string/resources/medication.ts(3 hunks)packages/core/src/external/fhir/export/string/resources/observation.ts(2 hunks)packages/core/src/external/fhir/export/string/resources/organization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/patient.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/practitioner.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/procedure.ts(2 hunks)packages/core/src/external/fhir/export/string/resources/related-person.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/address.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/age.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/annotation.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/attachment.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/code-display.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts(4 hunks)packages/core/src/external/fhir/export/string/shared/coding.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/deny.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/human-name.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/identifier.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/narrative.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/period.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/quantity.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/range.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/telecom.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/fhir/export/string/shared/quantity.tspackages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.tspackages/core/src/external/fhir/export/string/fhir-resource-to-string.tspackages/core/src/external/fhir/export/string/resources/procedure.tspackages/core/src/external/fhir/export/string/shared/period.tspackages/core/src/external/fhir/export/string/shared/identifier.tspackages/core/src/external/fhir/export/string/resources/medication.tspackages/core/src/external/fhir/export/string/resources/composition.tspackages/core/src/external/fhir/export/string/shared/annotation.tspackages/core/src/external/fhir/export/string/resources/medication-dispense.tspackages/core/src/external/fhir/export/string/resources/communication.tspackages/core/src/external/fhir/export/string/shared/code-display.tspackages/core/src/external/fhir/export/string/resources/encounter.tspackages/core/src/external/fhir/export/string/shared/telecom.tspackages/core/src/external/fhir/export/string/shared/age.tspackages/core/src/external/fhir/export/string/shared/duration.tspackages/core/src/external/fhir/export/string/resources/medication-statement.tspackages/core/src/external/fhir/export/string/shared/human-name.tspackages/core/src/external/fhir/export/string/resources/immunization.tspackages/core/src/external/fhir/export/string/resources/family-member-history.tspackages/core/src/external/fhir/export/string/shared/narrative.tspackages/core/src/external/fhir/export/string/resources/practitioner.tspackages/core/src/external/fhir/export/string/resources/patient.tspackages/core/src/external/fhir/export/string/resources/location.tspackages/core/src/external/fhir/export/string/resources/organization.tspackages/core/src/external/fhir/export/string/shared/deny.tspackages/core/src/external/fhir/export/string/resources/consent.tspackages/core/src/external/fhir/export/string/resources/diagnostic-report.tspackages/core/src/external/fhir/export/string/bundle-to-string.tspackages/core/src/external/fhir/export/string/resources/medication-administration.tspackages/core/src/external/fhir/export/string/shared/attachment.tspackages/core/src/external/fhir/export/string/resources/coverage.tspackages/core/src/external/fhir/export/string/shared/address.tspackages/core/src/external/fhir/export/string/resources/document-reference.tspackages/core/src/external/fhir/export/string/resources/allergy-intolerance.tspackages/core/src/external/fhir/export/string/resources/related-person.tspackages/core/src/external/fhir/export/string/shared/reference.tspackages/core/src/external/fhir/export/string/shared/codeable-concept.tspackages/core/src/external/fhir/export/string/shared/coding.tspackages/core/src/external/fhir/export/string/shared/dosage.tspackages/core/src/external/fhir/export/string/resources/condition.tspackages/core/src/external/fhir/export/string/resources/observation.tspackages/core/src/external/fhir/export/string/shared/range.tspackages/core/src/external/fhir/export/string/resources/medication-request.ts
🧬 Code Graph Analysis (31)
packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts (1)
packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcept(15-58)
packages/core/src/external/fhir/export/string/resources/procedure.ts (2)
packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcept(15-58)
packages/core/src/external/fhir/export/string/shared/period.ts (1)
packages/core/src/domain/patient.ts (1)
Period(18-26)
packages/core/src/external/fhir/export/string/resources/medication.ts (2)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (1)
FHIRResourceToString(8-15)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/composition.ts (7)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(7-18)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(9-15)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(9-22)
packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/medication-dispense.ts (5)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(7-18)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(9-14)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(4-13)
packages/core/src/external/fhir/export/string/resources/communication.ts (6)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(7-18)packages/core/src/external/fhir/export/string/shared/attachment.ts (1)
formatAttachment(15-29)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/encounter.ts (5)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/deny.ts (1)
emptyIfDenied(9-15)packages/core/src/external/fhir/export/string/shared/coding.ts (1)
formatCoding(14-30)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)
packages/core/src/external/fhir/export/string/shared/telecom.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/age.ts (1)
packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(9-14)
packages/core/src/external/fhir/export/string/shared/duration.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/medication-statement.ts (4)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(4-13)
packages/core/src/external/fhir/export/string/shared/human-name.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/family-member-history.ts (5)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(9-15)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(4-13)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/patient.ts (4)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (1)
FHIRResourceToString(8-15)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/human-name.ts (1)
formatHumanNames(11-22)packages/core/src/external/fhir/export/string/shared/address.ts (1)
formatAddresses(9-19)
packages/core/src/external/fhir/export/string/resources/location.ts (7)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcepts(66-77)formatCodeableConcept(15-58)packages/core/src/external/fhir/export/string/shared/telecom.ts (1)
formatTelecoms(9-19)packages/core/src/external/fhir/export/string/shared/address.ts (1)
formatAddress(21-34)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReference(20-24)
packages/core/src/external/fhir/export/string/resources/organization.ts (4)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (1)
FHIRResourceToString(8-15)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/address.ts (1)
formatAddresses(9-19)
packages/core/src/external/fhir/export/string/resources/medication-administration.ts (6)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(9-15)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(7-18)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/attachment.ts (2)
packages/core/src/util/mime.ts (1)
TXT_MIME_TYPE(39-39)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/coverage.ts (8)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcepts(66-77)formatCodeableConcept(15-58)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(7-18)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(9-15)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(9-14)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(9-22)
packages/core/src/external/fhir/export/string/shared/address.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts (6)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(7-18)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(4-13)
packages/core/src/external/fhir/export/string/resources/related-person.ts (7)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(66-77)packages/core/src/external/fhir/export/string/shared/human-name.ts (1)
formatHumanNames(11-22)packages/core/src/external/fhir/export/string/shared/telecom.ts (1)
formatTelecoms(9-19)packages/core/src/external/fhir/export/string/shared/address.ts (1)
formatAddresses(9-19)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/reference.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/coding.ts (3)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/deny.ts (2)
isAllowedSystem(18-20)emptyIfDenied(9-15)packages/core/src/external/fhir/export/string/shared/code-display.ts (1)
codeAndDisplayToString(1-9)
packages/core/src/external/fhir/export/string/shared/dosage.ts (3)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcepts(66-77)formatCodeableConcept(15-58)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(9-14)
packages/core/src/external/fhir/export/string/resources/condition.ts (10)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-15)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(4-6)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcepts(66-77)formatCodeableConcept(15-58)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(20-24)formatReferences(7-18)packages/core/src/external/fhir/export/string/shared/age.ts (1)
formatAge(8-10)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(9-15)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(9-24)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(4-13)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(9-22)
packages/core/src/external/fhir/export/string/resources/observation.ts (3)
packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcepts(66-77)formatCodeableConcept(15-58)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(9-14)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/range.ts (2)
packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(9-14)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
🪛 Biome (1.9.4)
packages/core/src/external/fhir/export/string/resources/coverage.ts
[error] 91-103: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/external/fhir/export/string/shared/dosage.ts
[error] 21-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (139)
packages/core/src/external/fhir/export/string/shared/telecom.ts (1)
1-19: The implementation looks good!The
formatTelecomsfunction correctly handles the array of contact points, applies proper filtering, and uses consistent formatting with optional labeling. The early returns for edge cases are well implemented.packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (1)
3-3: Appropriate standardization of minimum data flagThe introduction of this constant is a good refactoring practice to standardize behavior across multiple resource converters, replacing previously local boolean flags with a centralized value.
packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts (1)
49-56: Test correctly updated to reflect new behaviorThe test name and expectation have been appropriately updated to reflect the new more permissive handling of coding systems, where any system is now accepted rather than just predefined ones.
packages/core/src/external/fhir/export/string/shared/age.ts (1)
4-7: Good documentation explaining the specialized nature of AgeThe JSDoc comment clearly explains that Age is a special case of Quantity with time units under the UCUM system, which helps developers understand the context and relationship between these types.
packages/core/src/external/fhir/export/string/shared/period.ts (2)
9-11: Approved change to return undefined instead of empty string for missing dataThe updated function now returns
undefinedwhen period data is missing or incomplete, which is a more semantically correct approach than returning an empty string. This change aligns with broader formatting utility enhancements across the codebase.
14-14: Good addition of conditional label formattingMaking the label parameter optional and conditionally including it in the output improves the flexibility of this function when used across different contexts.
packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
4-4: Good use of constant for filter patternsDefining the HTML tags to filter as a constant array is a good approach for maintainability.
packages/core/src/external/fhir/export/string/resources/patient.ts (5)
2-2: Import path updated correctly.The import path change from "../types" to "../fhir-resource-to-string" aligns with the actual file location, making imports more consistent across the codebase.
4-6: Good import organization.Imports are now properly organized and include new utilities for formatting human names and identifiers, which aligns with the functional programming approach recommended in the coding guidelines.
12-12: Type signature improvement.Changing the return type from
stringtostring | undefinedcorrectly reflects that the method may return undefined when no meaningful string representation can be produced.
16-16: Clean code refactoring of conditional blocks.The refactoring to use concise single-line conditionals improves readability and follows the functional programming style recommended in the coding guidelines.
Also applies to: 19-19, 22-22, 24-24, 26-26
21-21: Added meaningful label parameter.Adding the "Address" label to formatAddresses improves the clarity of the output string, making it more searchable and user-friendly.
packages/core/src/external/fhir/export/string/resources/immunization.ts (6)
2-3: Good utility function imports.The imports now include defaultHasMinimumData, formatAnnotations, and emptyIfDenied, which promotes code reuse and consistency across the codebase.
Also applies to: 5-5
17-17: Proper initialization of minimum data flag.The hasMinimumData flag is correctly initialized with the defaultHasMinimumData value, establishing a baseline requirement for output generation.
32-32: Good minimum data tracking.Setting hasMinimumData to true when key fields (vaccineCode, lotNumber, note) are present ensures that the output string contains meaningful information.
Also applies to: 55-55, 92-92
52-53: Improved sanitization with emptyIfDenied.Using emptyIfDenied instead of checkDeny for sanitizing the lotNumber provides a more consistent approach to handling potentially denied fields.
89-93: Good use of shared utility for notes.Replacing manual note formatting with the formatAnnotations utility reduces code duplication and ensures consistent formatting across the codebase.
95-96: Proper minimum data check before return.The explicit check for minimum data before returning ensures that the method only returns a string when meaningful information is available.
packages/core/src/external/fhir/export/string/shared/range.ts (4)
1-4: Well-organized imports.The imports are correctly organized and include only what's necessary for the function to work.
5-8: Clear documentation.The JSDoc comment clearly explains the purpose of the formatRange function and the structure of a FHIR Range.
9-24: Well-implemented formatting utility.The formatRange function is well implemented, following the functional programming style recommended in the coding guidelines. It correctly:
- Handles undefined input
- Uses formatQuantity for low and high values
- Returns undefined if no parts are available
- Properly handles the optional label
14-15: Good error handling for empty quantities.The function correctly handles the case where low or high quantities might be missing by using the formatQuantity function, which returns undefined for invalid quantities.
Also applies to: 17-18
packages/core/src/external/fhir/export/string/resources/procedure.ts (3)
2-2: Import path updated correctly.The import path change from "../types" to "../fhir-resource-to-string" aligns with the actual file location, making imports more consistent across the codebase.
13-13: Type signature improvement.Changing the return type from
stringtostring | undefinedcorrectly reflects that the method may return undefined when no meaningful string representation can be produced.
39-44: Commented out subject field processing.The commented-out code explicitly indicates that the subject field is intentionally excluded from the string conversion, which aligns with the PR objective of clarifying excluded properties.
packages/core/src/external/fhir/export/string/resources/medication-statement.ts (5)
2-3: Updated imports to use the improved shared utilities.The code now correctly imports
defaultHasMinimumDataandformatAnnotationsfrom their respective modules, improving modularity and code reuse.
16-16: Standardized approach to track minimum data requirements.Using
defaultHasMinimumDatainstead of a local boolean ensures consistent behavior across all FHIR resource converters.
34-34: Proper tracking of minimum data presence for medication information.Setting the flag to true when medication data is present properly indicates that essential information is available for meaningful string output.
80-84: Improved note formatting with reusable utility.Using the shared
formatAnnotationsfunction reduces code duplication and ensures consistent formatting across resources. Also correctly tracks minimum data presence when notes are available.
86-86: Added safeguard to prevent output of incomplete data.Returning
undefinedwhen minimum data requirements aren't met prevents generating meaningless strings, aligning with the interface's return type.packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
9-14: Enhanced quantity formatting with better null handling and optional labeling.Good improvements to the
formatQuantityfunction:
- Made
labelparameter optional for more flexible usage- Return
undefinedinstead of empty string when quantity data is missing- Improved null safety with fallback to empty object
- More efficient string concatenation for the quantity value and unit
- Conditional label prefix only when a label is provided
The changes align with functional programming principles by making the function more deterministic and improving its handling of edge cases.
packages/core/src/external/fhir/export/string/resources/organization.ts (3)
2-5: Updated imports to use correct module locations.Correctly importing from the authoritative module locations improves maintainability.
11-11: Updated return type to match interface specification.Changed return type from
stringtostring | undefinedto align with the interface definition, allowing for the case when insufficient data is present.
35-36: Refactored address formatting using shared utility.Replaced inline address formatting with the shared
formatAddressesutility, which improves code consistency, reduces duplication, and handles edge cases better. The conditional push ensures we don't add empty entries to the parts array.packages/core/src/external/fhir/export/string/resources/composition.ts (7)
1-9: Well-structured imports for FHIR Composition handling.The imports are organized properly, bringing in the necessary FHIR types and shared formatting utilities needed for Composition resources.
10-16: Good implementation of the FHIR resource conversion interface.The class correctly implements
FHIRResourceToString<Composition>and initializes the minimum data tracking with the standarddefaultHasMinimumDataconstant.
27-33: Proper handling of type information with minimum data tracking.The code correctly formats the type information and marks it as minimum data, ensuring we only generate string output when meaningful data is present.
40-43: Commented out sections indicate intentional exclusions from search index.The commented code for subject and confidentiality information clearly shows which properties are intentionally excluded from the string representation, as mentioned in the PR description.
Also applies to: 80-82
66-66: Consider whether attestations should be considered minimum data.There's a commented-out line that would set
hasMinimumData = truefor attestations. Was this intentional? Verify whether attestation information should be considered sufficient for minimum data requirements.
95-106: Well-structured section handling with proper filtering.The implementation for handling Composition sections is well-organized, filtering out empty values and only adding to the parts array when there are actual sections to display.
108-110: Proper minimum data check before returning.The code correctly returns
undefinedwhen minimum data requirements aren't met, preventing the generation of meaningless strings. This is consistent with the approach used in other FHIR resource converters.packages/core/src/external/fhir/export/string/resources/medication.ts (4)
2-7: Good reorganization of importsThe imports have been properly reorganized to improve readability and maintainability. Adding the FIELD_SEPARATOR import ensures consistent formatting across all FHIR resource string converters.
13-13: Return type alignment with interface standardThe return type has been correctly updated to
string | undefinedto match the interface pattern introduced across other FHIR resource converters. This indicates that the method can return undefined when minimum required data is not present.
64-64: Consistent separator usage for ingredient componentsReplacing the hardcoded separator with the shared FIELD_SEPARATOR constant improves consistency across the codebase.
83-83: Consistent separator usage for batch partsUsing the FIELD_SEPARATOR constant instead of hardcoded string improves consistency.
packages/core/src/external/fhir/export/string/resources/location.ts (5)
1-9: Well-organized importsThe imports are properly organized, following the standard pattern used across other similar files. Good use of the shared formatting utilities.
14-17: Good initialization with minimum data checkThe implementation correctly initializes the hasMinimumData flag with the defaultHasMinimumData constant, following the pattern used in other resource converters.
65-67: Proper minimum data check before returningThe method correctly checks if minimum data is available before returning the string representation, returning undefined otherwise. This aligns with the pattern introduced across other FHIR resource converters.
10-68: Well-implemented LocationToString classThe class implementation is thorough and follows a consistent pattern for processing different parts of the Location resource. It properly handles optional fields and maintains the minimum data requirement check.
47-51:✅ Verification successful
Commented hasMinimumData assignment
There's a commented out hasMinimumData assignment for telecoms. This suggests a deliberate decision to not count telecoms toward minimum data requirements.
🌐 Web query:
Is telecom information typically required for FHIR Location resources?💡 Result:
In FHIR (Fast Healthcare Interoperability Resources), the Location resource is used to represent physical places where healthcare services are provided, such as hospitals, clinics, or wards.
Telecom information, which usually includes contact details like phone numbers, email addresses, and other communication means, can be included in the Location resource, but it is not typically mandatory. The telecom element in the Location resource is optional and is used to provide contact details for the location.
This means that while it's common and useful to include telecom information for accessibility and communication purposes, it is not strictly required by the FHIR specification. The requirement for telecom information might depend on specific implementation guides or use cases within different healthcare organizations or systems.
If you want, I can provide more detailed information or examples of how telecom information is represented in the FHIR Location resource. Would you like that?
Telecom on FHIR Location Is Optional; No Change Needed
According to the FHIR specification, the
telecomelement in a Location resource is optional, so it’s correct that it isn’t counted toward the minimum-data flag.• File:
packages/core/src/external/fhir/export/string/resources/location.ts (lines 47–51)const telecoms = formatTelecoms(location.telecom, "Telecom"); if (telecoms) { parts.push(telecoms); // hasMinimumData = true; }No further action required.
packages/core/src/external/fhir/export/string/resources/practitioner.ts (3)
1-7: Good reorganization of importsThe imports have been properly reorganized to use the new shared formatting functions.
13-13: Return type alignment with interface standardThe return type has been correctly updated to
string | undefinedto match the interface pattern.
16-30: Good use of shared formatting functionsThe implementation has been improved to use the shared formatting functions for human names, telecoms, and addresses, which promotes code reuse and consistency.
packages/core/src/external/fhir/export/string/resources/encounter.ts (5)
2-5: Appropriate modularization of imports and dependencies.The update to imports introduces essential modules for the refactored functionality, following the package's modular approach.
15-15: Improved explicit naming with standardized initialization.Renaming from
hasRelevantDatatohasMinimumDataand using the shareddefaultHasMinimumDataconstant provides clearer semantics and better standardization across resource converters.
23-27: Enhanced data sanitization with improved function name.The use of
emptyIfDeniedinstead of a previous function provides more intuitive naming and standardizes how denied values are handled across resources.
30-34: More precise formatting with specialized utility.Switching from
formatCodeableConcepttoformatCodingfor the Encounter class field allows for more granular control and appropriate type-specific formatting.
49-49: Consistent data validation with standardized pattern.Adding a minimum data check before returning ensures that resources with insufficient data return
undefinedrather than empty or meaningless strings, improving consistency across resource converters.packages/core/src/external/fhir/export/string/shared/attachment.ts (2)
1-9: Well-structured module with clear type constraints.The module establishes a focused scope for handling attachment conversions with explicit limitations on content types. The comment on lines 6-7 provides good context for future expansion possibilities.
31-42: Efficient functional implementation for collection processing.The implementation follows functional programming principles with appropriate filtering of empty values. Good use of optional chaining and nullish checks to handle potentially undefined inputs.
packages/core/src/external/fhir/export/string/resources/medication-dispense.ts (5)
2-8: Appropriate modularization with consolidated imports.The imports are well-organized and include all necessary utilities for the updated implementation.
14-16: Improved function signature with standardized minimum data tracking.Updating the return type to include
undefinedand adding thehasMinimumDataflag follows the consistent pattern used across resource converters, ensuring conformity in the API.
42-43: Clear criteria for minimum data requirements.Setting
hasMinimumDatato true when either medication information or notes are present establishes explicit criteria for what constitutes valid output.Also applies to: 131-132
128-132: Improved note formatting with specialized utility.Using the
formatAnnotationsutility instead of manual string concatenation standardizes the formatting across resources and leverages shared utilities effectively.
134-135: Consistent data validation with standardized pattern.Adding a minimum data check before returning aligns with the pattern used across other resource converters, ensuring a uniform approach to handling insufficient data.
packages/core/src/external/fhir/export/string/shared/deny.ts (2)
6-9: Improved function documentation.Adding a clear JSDoc comment explains the purpose and behavior of the
emptyIfDeniedfunction, making the code more maintainable.
9-15: More descriptive function name.Renaming from
checkDenytoemptyIfDeniedbetter describes the function's behavior of returningundefinedfor denied values.packages/core/src/external/fhir/export/string/resources/communication.ts (3)
1-101: Implementation looks solid and follows best practices.The
CommunicationToStringclass properly implements theFHIRResourceToStringinterface and follows a consistent pattern for converting FHIR resources to string representations. The code is well-structured with logical blocks for each field, and properly handles the minimum data presence check.
35-38: Commented-out subject field aligns with PR objectives.This commented-out block explicitly indicates that the subject field is intentionally excluded from the string conversion, which matches the PR description stating that commented-out code sections indicate which properties are excluded from search indexing.
79-96: Payload processing logic is thorough and well-implemented.The code correctly extracts different types of payload content (string, attachment, reference) and properly handles filtering and combining them. Setting
hasMinimumData = truewhen payloads exist ensures that resources with meaningful payload content are included in search results.packages/core/src/external/fhir/export/string/bundle-to-string.ts (2)
4-26: Good extension of resource type imports.The file has been enhanced to support many more FHIR resource types, which aligns with the PR objective of improving FHIR resource to string conversion coverage.
148-155: Well-implemented utility function for individual resource conversion.The new
resourceToStringfunction follows the same pattern asbundleToStringbut for a single resource, which is a logical and useful addition. It properly handles validation and error cases by returning undefined when appropriate.packages/core/src/external/fhir/export/string/shared/annotation.ts (2)
4-13: Well-structured and reusable formatting utility.The
formatAnnotationsfunction follows functional programming principles with clear input/output, proper validation, and consistent error handling. The implementation aligns with the coding guidelines by being deterministic and avoiding state modification.
15-28: Clean implementation of single annotation formatting.The
formatAnnotationfunction correctly extracts and formats the relevant fields from an Annotation object. The conditional handling of the optional label parameter is implemented consistently with other formatting utilities.packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts (5)
2-7: Good refactoring of imports for better modularity.The import statements have been updated to use the newly introduced utilities, which improves code organization and reduces duplication.
14-15: Consistent initialization of minimum data flag.Using
defaultHasMinimumDatato initialize the flag is consistent with the pattern used across other resource converters, improving maintainability.
42-43: Improvement in separator consistency.Changing to use the
FIELD_SEPARATORconstant instead of hardcoding the separator improves maintainability and consistency across the codebase.
84-88: Good refactoring to use shared annotation formatting.Replacing custom note formatting with the shared
formatAnnotationsutility improves consistency and reduces code duplication across resource converters.
90-90: Consistent handling of minimum data check.The check for minimum data presence and early return is consistent with the interface contract and other resource converters. This ensures that only resources with meaningful searchable content are included.
packages/core/src/external/fhir/export/string/resources/related-person.ts (7)
1-7: LGTM: Imports, dependencies, and organization are well-structured.The imports are organized clearly and the file follows the appropriate dependency structure.
10-14: Implementation adheres to the FHIR Resource interface.The class properly implements the
FHIRResourceToString<RelatedPerson>interface with appropriate JSDoc comments.
14-17: LGTM: Boolean flag pattern for minimum data validation.The initialization of
hasMinimumDatawith the default value and collection of parts array follows the established pattern.
23-32: LGTM: Good implementation of relationship as minimum data.Setting
hasMinimumData = truewhen relationship data is available is a reasonable criterion for determining what constitutes sufficient information for this resource type.
49-64: Commented-out code clarifies exclusion decisions.The commented-out code for the
photoproperty clearly documents which properties are intentionally excluded from the search index, aligning with the PR objectives.
66-81: Commented-out code clarifies exclusion decisions.The commented-out code for the
communicationproperty clearly documents which properties are intentionally excluded from the search index, aligning with the PR objectives.
83-86: LGTM: Proper handling of minimum data requirement.The function correctly returns undefined when minimum data requirements aren't met, ensuring that only meaningful strings are returned.
packages/core/src/external/fhir/export/string/resources/consent.ts (7)
1-16: LGTM: Well-organized imports.The imports are logically grouped and include all necessary dependencies for the class.
20-24: LGTM: Follows established pattern for resource string conversion.The class properly implements the
FHIRResourceToString<Consent>interface and follows the established pattern with the minimum data boolean flag.
36-39: Commented-out code for patient references.The commented-out code for patient references clearly indicates it's excluded from search indexing, which aligns with the PR objectives.
49-55: LGTM: Good criterion for minimum data presence.Setting
hasMinimumData = truewhen source attachment is present makes sense as a minimum data criterion.
60-75: LGTM: Comprehensive policy formatting.The policy formatting is well-structured and correctly sets
hasMinimumData = truewhen policies are present.
153-168: Commented-out code for sub-provisions.The commented-out code for sub-provisions clearly indicates this feature is excluded from the current implementation, aligning with the PR objectives.
179-182: LGTM: Properly handles minimum data requirement.The function correctly returns undefined when no minimum data is present, ensuring that only meaningful strings are returned.
packages/core/src/external/fhir/export/string/resources/coverage.ts (5)
1-10: LGTM: Well-organized imports.The imports are logically grouped and include all necessary dependencies for the class.
14-18: LGTM: Class follows established pattern.The class properly implements the
FHIRResourceToString<Coverage>interface and follows the established pattern with the minimum data boolean flag.
48-51: Commented-out code clarifies exclusion decisions.The commented-out code for
subscriberId,order,network, andsubrogationproperties clearly documents which properties are intentionally excluded from the search index, aligning with the PR objectives.
68-82: LGTM: Good criterion for minimum data presence.Setting
hasMinimumData = truewhen class data is present makes sense as a minimum data criterion.
123-126: LGTM: Properly handles minimum data requirement.The function correctly returns undefined when no minimum data is present, ensuring that only meaningful strings are returned.
packages/core/src/external/fhir/export/string/shared/human-name.ts (4)
4-5: LGTM: Good constant definition.Defining
NAME_SEPARATORas a constant follows good practices for maintainability.
11-14: LGTM: Improved function signature with optional label.Adding an optional label parameter increases the flexibility of this utility function.
17-22: LGTM: Good implementation of filtering and formatting.Filtering out falsy values and joining with the appropriate separator is a good pattern.
24-37: 🛠️ Refactor suggestionAdd explicit null check for name.given.
There's no explicit null check before calling
join()onname.given, which could cause errors if given is null.- const required = [name.given?.join(NAME_SEPARATOR), name.family].filter(Boolean); + const givenName = name.given?.length ? name.given.join(NAME_SEPARATOR) : undefined; + const required = [givenName, name.family].filter(Boolean);Likely an incorrect or invalid review comment.
packages/core/src/external/fhir/export/string/resources/document-reference.ts (6)
1-9: Good use of shared formatting utilities for modular code.The imports leverage shared formatting utilities which promotes code reuse and consistency across different resource formatters.
13-16: Implementation follows the interface correctly.The class correctly implements the
FHIRResourceToString<DocumentReference>interface and initializes the minimum data flag from the default value.
87-90: Description contributes to minimum data presence - good.Setting
hasMinimumData = truewhen description is present is appropriate, as the description provides meaningful searchable content.
97-112: Content exclusion aligns with PR objectives.The commented-out content section aligns with the PR's objective of explicitly indicating which properties are excluded from the string conversion process, clarifying the scope of searchable content.
114-146: Context formatting is thorough and well-structured.The context section appropriately formats multiple nested fields (encounter, events, period, facility, practice) while maintaining clear structure with proper labels.
151-154: Minimum data check ensures meaningful output.The check for minimum data before returning the string ensures that only sufficiently meaningful content is provided, preventing empty or low-value strings from being returned.
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (9)
3-9: Good refactoring with clear function naming.The change from
checkDenytoemptyIfDeniedimproves code readability and better describes the function's purpose.
17-17: Consistent use of minimum data flag.Using
defaultHasMinimumDataas the initial value maintains consistency with other resource formatters.
23-26: Commented-out section for basedOn follows PR pattern.The commented-out section for
basedOnaligns with the PR's objective of explicitly indicating excluded properties.
28-31: Enhanced data validation with emptyIfDenied.Using
emptyIfDeniedfor status ensures proper data validation before including it in the output.
57-62: Improved date formatting with clear labels.The change to "Effective Date" and using
formatPeriodimproves clarity and consistency of date representation.
66-93: Good addition of additional reference fields.The addition of
imagingStudy,performer,resultsInterpreter, andspecimenformatting improves the completeness of the string representation.
108-111: Enhancement with conclusionCode formatting.Adding formatting for
conclusionCodeusingformatCodeableConceptsprovides additional context in the string output.
113-114: Narrative text inclusion improves search relevance.Adding the formatted narrative text will likely enhance search relevance, aligning with the PR's objective.
119-119: Consistent minimum data check.The minimum data check before returning ensures consistency with other resource formatters.
packages/core/src/external/fhir/export/string/resources/medication-administration.ts (5)
12-14: Class naming and interface implementation follow conventions.The class name uses PascalCase and properly implements the
FHIRResourceToStringinterface for the specific resource type.
24-27: Status field contributes to minimum data.Setting
hasMinimumData = truewhen status is present is appropriate since status is a core element for medication administrations.
35-50: Good handling of medication representation alternatives.The code correctly handles both
medicationCodeableConceptandmedicationReferencealternatives, with proper minimum data flagging for both cases.
91-105: Comprehensive dosage information capture.The implementation properly formats multiple dosage components (route, method, dose, rate, text) into a structured string representation.
107-110: Consistent minimum data check and string concatenation.The minimum data check and string joining with the field separator maintain consistency with other resource formatters.
packages/core/src/external/fhir/export/string/resources/family-member-history.ts (5)
16-18: Intentional exclusion of identifiers.The commented-out code for identifiers aligns with the PR's approach of explicitly indicating excluded properties.
32-43: Appropriate minimum data qualification for core fields.Setting
hasMinimumData = truefor name and relationship is appropriate as these are core identifying elements for family member history.
50-57: Good handling of birth information alternatives.The code properly handles the three alternative birth representation formats (period, date, string).
88-109: Comprehensive condition formatting.The implementation creates a detailed string representation of conditions, including code, outcome, onset, and notes, which helps create rich searchable content.
111-114: Consistent minimum data check and string joining.The code follows the established pattern for minimum data checking and string joining with field separators.
packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (4)
15-18: Good addition of optional label parameterAdding an optional
labelparameter while keeping the return type asstring | undefinedmaintains backward compatibility. This change enhances reusability of the function across different contexts, and aligns with the functional programming principles from the coding guidelines.
29-36: Improved modularity through helper functionsThe use of
getCodeandformatCodinghelper functions instead of direct property access enhances modularity and code reuse. This change follows the coding guidelines that encourage building small, deterministic functions and minimizing coupling.
38-42: More consistent handling of code and displayMoving from inline string formatting to the
codeAndDisplayToStringhelper function improves consistency and maintainability. Code is now easier to read and maintain, aligning with the coding guidelines.
72-73: Consistent null checking with filter(Boolean)Using
.filter(Boolean)to remove undefined values is a concise pattern that aligns with functional programming principles in the coding guidelines. This ensures that we don't include any undefined values in the output.packages/core/src/external/fhir/export/string/shared/reference.ts (2)
13-14: Good use of functional programming patternThe use of
mapandfilter(Boolean)follows functional programming principles from the coding guidelines. This pattern is concise and expressive, making it clear that we're transforming and filtering the references.
20-24: Well-factored implementation of formatReferenceThe implementation of
formatReferencefollows the coding guidelines of creating small, focused functions with a single responsibility. It properly handles undefined inputs, trims the display text, and conditionally includes a label. This improves code reuse and maintainability.packages/core/src/external/fhir/export/string/resources/observation.ts (3)
28-31: Improved type-specific formattingUsing
formatCodeableConceptdirectly for a single concept instead of wrapping it in an array forformatCodeableConceptsis more type-accurate and efficient. This change better follows the principle of using the most specific function for the job.
49-55: Good separation of value typesSeparating the handling of
valueQuantityandvalueCodeableConceptimproves clarity and maintainability. Each value type now has its own formatting logic, making the code more modular and easier to understand.
57-60: Good addition of interpretation formattingAdding support for the
interpretationfield enhances the completeness of the string representation, which aligns with the PR objective of improving the quality of text output for search criteria.packages/core/src/external/fhir/export/string/shared/address.ts (2)
13-15: Improved consistency in return valuesChanging the return value to
undefinedfor empty or undefined inputs aligns with other formatting functions in this codebase. The additional check after filtering ensures we don't return an empty array of formatted addresses, which is a good defensive programming practice.
21-34: Well-implemented formatAddress helper functionThe new helper function follows the pattern established in other formatting utilities, making the code more modular and consistent. It properly handles undefined inputs, extracts relevant components, and supports optional labeling.
packages/core/src/external/fhir/export/string/resources/condition.ts (1)
55-60: Subject intentionally excluded — confirm downstream search behaviourThe subject reference is commented out. Omitting it removes a highly searchable field (patient name or encounter context). If this is temporary, please add a TODO with justification; otherwise consider re-enabling it.
packages/core/src/external/fhir/export/string/resources/composition.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/fhir/export/string/shared/identifier.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/fhir/export/string/shared/identifier.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/fhir/export/string/resources/location.ts
Outdated
Show resolved
Hide resolved
| export function formatAttachment(attachment: Attachment): string | undefined { | ||
| if (!attachment.data || !attachment.contentType) return undefined; | ||
| if (!conversibleContentTypes.includes(attachment.contentType)) return undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling for base64 decoding.
The function directly applies base64ToString without handling potential decoding errors that could occur with invalid base64 data.
- if (!attachment.data || !attachment.contentType) return undefined;
- if (!conversibleContentTypes.includes(attachment.contentType)) return undefined;
+ if (!attachment.data || !attachment.contentType) return undefined;
+ if (!conversibleContentTypes.includes(attachment.contentType)) return undefined;
+
+ // Safely decode base64 data with error handling
+ let decodedData: string;
+ try {
+ decodedData = base64ToString(attachment.data);
+ } catch (error) {
+ return undefined; // Skip attachments with invalid base64 data
+ }📝 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.
| export function formatAttachment(attachment: Attachment): string | undefined { | |
| if (!attachment.data || !attachment.contentType) return undefined; | |
| if (!conversibleContentTypes.includes(attachment.contentType)) return undefined; | |
| export function formatAttachment(attachment: Attachment): string | undefined { | |
| if (!attachment.data || !attachment.contentType) return undefined; | |
| if (!conversibleContentTypes.includes(attachment.contentType)) return undefined; | |
| // Safely decode base64 data with error handling | |
| let decodedData: string; | |
| try { | |
| decodedData = base64ToString(attachment.data); | |
| } catch (error) { | |
| return undefined; // Skip attachments with invalid base64 data | |
| } | |
| // …rest of function, e.g. return decodedData or process further | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/external/fhir/export/string/shared/attachment.ts around
lines 15 to 17, the function formatAttachment calls base64ToString on
attachment.data without handling errors that may arise from invalid base64
input. To fix this, wrap the base64ToString call in a try-catch block to catch
any decoding errors and handle them gracefully, such as returning undefined or
logging the error, to prevent the function from throwing exceptions on invalid
data.
There was a problem hiding this comment.
This is fine: we don't have the original string to compare against, so we can't tell if it's incorrect or not; any string we throw to the base64 encoder will be encoded.
There was a problem hiding this comment.
@leite08 You're right about the base64 encoding. From examining the code, I can see that base64ToString is a simple wrapper around Buffer.from(base64, "base64").toString("utf8"). If all attachment data in your system is properly base64-encoded (which is typically the case for FHIR resources), then decoding should work without issues.
While Node's Buffer.from() can theoretically throw on malformed inputs, in your context where attachments are properly created within the system, this is unlikely to occur. I'll respect your decision to keep the code as is.
✏️ Learnings added
Learnt from: leite08
PR: metriport/metriport#3869
File: packages/core/src/external/fhir/export/string/shared/attachment.ts:0-0
Timestamp: 2025-05-21T16:30:15.384Z
Learning: The base64ToString function in the Metriport codebase is used to decode base64-encoded data in FHIR resources. The team has determined that error handling for invalid base64 data is unnecessary since all base64 data in the system should be properly encoded, and there's no reference to compare against for validation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (condition.onsetDateTime) parts.push(`Onset Date: ${condition.onsetDateTime}`); | ||
|
|
||
| const ageStr = formatAge(condition.onsetAge); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Raw date/time values are output without formatting
Onset Date: ${condition.onsetDateTime} (and the analogous abatement / recorded dates) will emit the raw ISO string. For end-user readability and locale consistency we normally run these through the existing buildDayjs() + .format() helpers used elsewhere in the codebase.
-if (condition.onsetDateTime) parts.push(`Onset Date: ${condition.onsetDateTime}`);
+if (condition.onsetDateTime)
+ parts.push(`Onset Date: ${buildDayjs(condition.onsetDateTime).format("YYYY-MM-DD")}`);Apply the same treatment to abatement and recorded dates.
Also applies to: 77-78, 90-91
🤖 Prompt for AI Agents
In packages/core/src/external/fhir/export/string/resources/condition.ts around
lines 64 to 66, the raw ISO date strings for onsetDateTime, abatementDateTime,
and recordedDateTime are output directly without formatting, which reduces
readability and locale consistency. To fix this, wrap these date values with the
existing buildDayjs() function and apply the .format() method before including
them in the output strings. Make sure to apply this change not only at lines
64-66 but also similarly at lines 77-78 and 90-91 for the corresponding date
fields.
There was a problem hiding this comment.
that's the expected behavior
There was a problem hiding this comment.
@leite08 I understand that using the raw ISO date strings is the expected behavior. Thanks for clarifying!
| const medicationStr = formatCodeableConcept(request.medicationCodeableConcept, "Medication"); | ||
| if (medicationStr) { | ||
| parts.push(medicationStr); | ||
| hasMinimumData = true; | ||
| } | ||
|
|
||
| // Add requester | ||
| if (request.requester) { | ||
| const requesterStr = formatReferences([request.requester], "Requester"); | ||
| if (requesterStr) { | ||
| parts.push(requesterStr); | ||
| } | ||
| } | ||
| const medicationRefStr = formatReference(request.medicationReference, "Medication"); | ||
| if (medicationRefStr) parts.push(medicationRefStr); | ||
|
|
There was a problem hiding this comment.
hasMinimumData is not updated when medicationReference is present
hasMinimumData is set to true when medicationCodeableConcept is formatted (lines 50-53) but not when the request identifies the medication solely via medicationReference.
If a resource contains only a reference and none of the other minimum-data fields, the method will incorrectly return undefined, silently dropping a valid record from any downstream search/index logic.
-const medicationRefStr = formatReference(request.medicationReference, "Medication");
-if (medicationRefStr) parts.push(medicationRefStr);
+const medicationRefStr = formatReference(request.medicationReference, "Medication");
+if (medicationRefStr) {
+ parts.push(medicationRefStr);
+ hasMinimumData = true;
+}📝 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.
| const medicationStr = formatCodeableConcept(request.medicationCodeableConcept, "Medication"); | |
| if (medicationStr) { | |
| parts.push(medicationStr); | |
| hasMinimumData = true; | |
| } | |
| // Add requester | |
| if (request.requester) { | |
| const requesterStr = formatReferences([request.requester], "Requester"); | |
| if (requesterStr) { | |
| parts.push(requesterStr); | |
| } | |
| } | |
| const medicationRefStr = formatReference(request.medicationReference, "Medication"); | |
| if (medicationRefStr) parts.push(medicationRefStr); | |
| const medicationStr = formatCodeableConcept(request.medicationCodeableConcept, "Medication"); | |
| if (medicationStr) { | |
| parts.push(medicationStr); | |
| hasMinimumData = true; | |
| } | |
| const medicationRefStr = formatReference(request.medicationReference, "Medication"); | |
| if (medicationRefStr) { | |
| parts.push(medicationRefStr); | |
| hasMinimumData = true; | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/external/fhir/export/string/resources/medication-request.ts
between lines 49 and 57, the variable hasMinimumData is set to true only when
medicationCodeableConcept is present but not when medicationReference is
present. To fix this, update hasMinimumData to true also when
medicationReference is formatted and added to parts, ensuring valid records with
only medicationReference are correctly recognized as having minimum data.
There was a problem hiding this comment.
it's fine for now, we'll review these later on and can add it if needed
There was a problem hiding this comment.
@leite08 Understood! I'll note that this is deferred for a later review. Let me know if you need any assistance when you're ready to revisit this issue.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
packages/core/src/external/fhir/export/string/resources/medication-administration.ts (1)
76-76: Inconsistent minimum data flagging for date.The
hasMinimumDataflag assignment foreffectiveDateTimeis commented out, while it's active foreffectivePeriod(lines 33-36). This creates inconsistency in how temporal data contributes to minimum data presence.if (administration.effectiveDateTime) { parts.push( isDebug ? `Effective Date: ${administration.effectiveDateTime}` : administration.effectiveDateTime ); - // hasMinimumData = true; + hasMinimumData = true; }packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
66-66:⚠️ Potential issueImprove text deduplication logic.
The current text deduplication logic using a simple
includescheck can cause false positives with substring matching. For instance, if text is "COLD" and coding includes "COMMON COLD", the text would be incorrectly considered a duplicate.Consider using a more precise matching approach:
- const textStr = text && !codingStr.includes(text) ? `${text}${SEP_BETWEEN_TEXT_AND_CODING} ` : ""; + // Check for exact matches or matches with appropriate separators to avoid substring false positives + const isDuplicate = text && ( + codingStr === text || + codingStr.includes(`${text}${SEP_BETWEEN_TEXT_AND_CODING}`) || + codingStr.includes(` ${SEP_BETWEEN_CODING} ${text}`) || + codingStr.includes(`${text} ${SEP_BETWEEN_CODING}`) + ); + const textStr = text && !isDuplicate ? `${text}${SEP_BETWEEN_TEXT_AND_CODING} ` : "";packages/core/src/external/fhir/export/string/resources/condition.ts (2)
82-84: 🛠️ Refactor suggestionDate/time values should be formatted for readability
Raw ISO date strings are being output without formatting, which reduces readability. Consider using the
buildDayjs()function with a consistent format.-if (condition.onsetDateTime) { - parts.push(isDebug ? `Onset Date: ${condition.onsetDateTime}` : condition.onsetDateTime); +if (condition.onsetDateTime) { + const formattedDate = buildDayjs(condition.onsetDateTime).format("YYYY-MM-DD"); + parts.push(isDebug ? `Onset Date: ${formattedDate}` : formattedDate); }This formatting should also be applied to other date fields (lines 103-107, 132-134).
18-19: 🛠️ Refactor suggestionConsider broader criteria for minimum data
The
hasMinimumDatais only set totruewhencondition.codeis present. This could exclude legitimate clinical statements that may have other important fields but no code.Consider broadening the criteria or making it configurable:
-let hasMinimumData = defaultHasMinimumData; +let hasMinimumData = defaultHasMinimumData; +// Set to true if any of the core clinical content is present +if (condition.code || condition.category?.length || + condition.bodySite?.length || condition.severity) { + hasMinimumData = true; +}Also applies to: 57-60
packages/core/src/external/fhir/export/string/resources/medication-request.ts (1)
69-74:⚠️ Potential issueNot setting
hasMinimumDatafor medicationReference
hasMinimumDatais set totruewhenmedicationCodeableConceptis present but not whenmedicationReferenceis present. This could cause valid records with only a medication reference to be incorrectly excluded.const medicationRefStr = formatReference({ reference: request.medicationReference, label: "Medication", isDebug, }); -if (medicationRefStr) parts.push(medicationRefStr); +if (medicationRefStr) { + parts.push(medicationRefStr); + hasMinimumData = true; +}packages/core/src/external/fhir/export/string/shared/dosage.ts (2)
39-39:⚠️ Potential issueTiming object rendered as [object Object]
The
timingproperty is a complex object, and direct string interpolation will result in[object Object]in the output. This produces unhelpful text in the search index.-timing && isDebug ? `Timing: ${timing}` : timing, +// Temporarily omit timing until a proper formatter is implemented +// timing && isDebug ? `Timing: ${timing}` : timing,Consider creating a dedicated
formatTimingfunction to properly format the timing object, similar to how other FHIR elements are handled.
60-64:⚠️ Potential issueComplex objects need dedicated formatters
The
doseRange,rateRatio, andrateRangeproperties are complex objects being directly string interpolated, which will result in[object Object]in the output.-doseRange && isDebug ? `Dose Range: ${doseRange}` : doseRange, +doseRange && formatRange({ range: doseRange, label: "Dose Range", isDebug }), doseQuantity && formatQuantity({ quantity: doseQuantity, label: "Dose", isDebug }), -rateRatio && isDebug ? `Rate Ratio: ${rateRatio}` : rateRatio, -rateRange && isDebug ? `Rate Range: ${rateRange}` : rateRange, +rateRatio && formatRatio({ ratio: rateRatio, label: "Rate Ratio", isDebug }), +rateRange && formatRange({ range: rateRange, label: "Rate Range", isDebug }),You'll need to import
formatRangefrom "../shared/range" and create a newformatRatiofunction or import it if available.
🧹 Nitpick comments (10)
packages/core/src/external/fhir/export/string/shared/telecom.ts (2)
31-52: Missing JSDoc for formatTelecom functionWhile the implementation is good, the function is missing JSDoc documentation to describe its purpose and parameters, unlike other functions in this file.
Add JSDoc documentation for the
formatTelecomfunction:+/** + * Formats a FHIR contact point into a string representation + * @param telecom - FHIR contact point to format + * @param label - Label to prefix the formatted string with + * @param isDebug - Whether to include the label in the output + * @returns Formatted string of the contact point + */ export function formatTelecom({
42-47: Commented-out code should be addressedThere are commented-out lines for formatting the
useandrankproperties. This seems intentional based on the PR objectives which mention "commented-out code sections that explicitly indicate which properties are excluded from the string conversion process."However, consider adding a comment explaining why these properties are excluded to make the intent clearer.
const components = [ system && isDebug ? `System: ${system}` : system, value && isDebug ? `Value: ${value}` : value, - // use && isDebug ? `Use: ${use}` : use, - // rank && isDebug ? `Rank: ${rank}` : rank, + // Explicitly excluding 'use' and 'rank' properties from search index + // use && isDebug ? `Use: ${use}` : use, + // rank && isDebug ? `Rank: ${rank}` : rank, ].filter(Boolean);packages/core/src/external/fhir/export/string/shared/duration.ts (1)
5-25: Incomplete JSDoc for formatDuration functionWhile the function implementation is good, the JSDoc is missing parameter descriptions for
labelandisDebug.Complete the JSDoc with parameter descriptions:
/** * Formats a FHIR Duration object into a string representation + * @param duration - FHIR Duration object to format + * @param label - Label to prefix the formatted string with + * @param isDebug - Whether to include the label in the output + * @returns Formatted string of the duration or undefined if no relevant data */ export function formatDuration({packages/core/src/external/fhir/export/string/shared/narrative.ts (2)
5-5: Consider making the filter array more maintainableWhile the current approach works, consider using a more maintainable approach for the filter array. For complex patterns, you might want to consider using a regex pattern that's easier to maintain or document what each pattern is filtering out.
-const filterFromDiv = ['<div xmlns="http://www.w3.org/1999/xhtml">', "<div>", "</div>", "<div/>"]; +// Common XHTML tags to remove from narrative content to get clean text +const filterFromDiv = [ + '<div xmlns="http://www.w3.org/1999/xhtml">', // XML namespace declaration + "<div>", // Opening div tag + "</div>", // Closing div tag + "<div/>" // Self-closing div tag +];
10-18: Inconsistent parameter requirementUnlike other formatting functions that make the
labelparameter optional, this function requires it. While this makes sense since the label is needed for formatting in debug mode, consider making it consistent with other formatters by making it optional for a more unified API.export function formatNarrative({ narrative, label, isDebug = defaultIsDebug, }: { narrative: Narrative | undefined; - label: string; + label?: string; isDebug?: boolean | undefined; }): string | undefined {Then update line 26 to handle the optional label:
- parts.push(isDebug ? `${label}: ${filteredDiv}` : filteredDiv); + parts.push(isDebug && label ? `${label}: ${filteredDiv}` : filteredDiv);packages/core/src/external/fhir/export/string/resources/medication-administration.ts (1)
58-61: Consider clarifying the exclusion of subject information.The subject reference is commented out - this appears to be intentional based on similar patterns in other FHIR resource converters, but documenting the reason would improve clarity.
// if (administration.subject) { // const subjectStr = formatReferences([administration.subject], "Subject"); // if (subjectStr) parts.push(subjectStr); // } +// Subject information is intentionally excluded from the string representation as per ENG-268packages/core/src/external/fhir/export/string/resources/medication-dispense.ts (2)
49-54: Consider clarifying the exclusion of subject information.The subject reference is commented out - this appears to be intentional based on similar patterns in other FHIR resource converters, but documenting the reason would improve clarity.
// if (dispense.subject) { // const subjectStr = formatReferences({ references: [dispense.subject], label: "Subject", isDebug }); // if (subjectStr) { // parts.push(subjectStr); // } // } +// Subject information is intentionally excluded from the string representation as per ENG-268
101-106: Consider clarifying the exclusion of daysSupply information.The daysSupply block is commented out without explanation. Adding a comment would improve code readability.
// if (dispense.daysSupply) { // const daysSupplyStr = formatQuantity({ quantity: dispense.daysSupply, label: "Days Supply", isDebug }); // if (daysSupplyStr) { // parts.push(daysSupplyStr); // } // } +// Days supply information is intentionally excluded from the string representation as per ENG-268packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (2)
48-59: Consider clarifying the exclusion of subject and encounter information.Multiple blocks are commented out (subject, encounter) without explanation. Adding comments would improve code readability.
// if (report.subject) { // const subjectStr = formatReferences([report.subject], "Subject"); // if (subjectStr) { // parts.push(subjectStr); // hasMinimumData = true; // } // } +// Subject information is intentionally excluded from the string representation as per ENG-268 // if (report.encounter) { // const encounterStr = formatReferences([report.encounter], "Encounter"); // if (encounterStr) parts.push(encounterStr); // } +// Encounter information is intentionally excluded from the string representation as per ENG-268
74-74: The issued field doesn't update hasMinimumData.The issued date is included in the output but doesn't increment the hasMinimumData flag. Consider whether this field should contribute to minimum data presence for consistency.
if (report.issued) parts.push(isDebug ? `Issued: ${report.issued}` : report.issued); +hasMinimumData = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (46)
packages/core/src/external/fhir/codeable-concept.ts(1 hunks)packages/core/src/external/fhir/export/string/bundle-to-string.ts(6 hunks)packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/communication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/composition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/condition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/consent.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/coverage.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/document-reference.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/encounter.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/family-member-history.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/immunization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/location.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-administration.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-request.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-statement.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/observation.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/organization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/patient.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/practitioner.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/procedure.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/related-person.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts(6 hunks)packages/core/src/external/fhir/export/string/shared/address.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/age.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/annotation.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/attachment.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/code-display.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts(2 hunks)packages/core/src/external/fhir/export/string/shared/coding.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/debug.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/human-name.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/identifier.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/narrative.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/period.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/quantity.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/range.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/telecom.ts(1 hunks)packages/core/src/external/fhir/normalization/resources/encounter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/external/fhir/normalization/resources/encounter.ts
- packages/core/src/external/fhir/export/string/shared/debug.ts
- packages/core/src/external/fhir/export/string/shared/annotation.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- packages/core/src/external/fhir/export/string/shared/tests/codeable-concept.test.ts
- packages/core/src/external/fhir/export/string/shared/age.ts
- packages/core/src/external/fhir/export/string/shared/code-display.ts
- packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts
- packages/core/src/external/fhir/export/string/resources/patient.ts
- packages/core/src/external/fhir/export/string/resources/composition.ts
- packages/core/src/external/fhir/export/string/resources/consent.ts
- packages/core/src/external/fhir/export/string/resources/location.ts
- packages/core/src/external/fhir/export/string/resources/medication.ts
- packages/core/src/external/fhir/export/string/resources/coverage.ts
- packages/core/src/external/fhir/export/string/resources/practitioner.ts
- packages/core/src/external/fhir/export/string/resources/document-reference.ts
- packages/core/src/external/fhir/export/string/resources/communication.ts
- packages/core/src/external/fhir/export/string/resources/related-person.ts
- packages/core/src/external/fhir/export/string/shared/human-name.ts
- packages/core/src/external/fhir/export/string/resources/procedure.ts
- packages/core/src/external/fhir/export/string/resources/immunization.ts
- packages/core/src/external/fhir/export/string/resources/encounter.ts
- packages/core/src/external/fhir/export/string/resources/medication-statement.ts
- packages/core/src/external/fhir/export/string/shared/address.ts
- packages/core/src/external/fhir/export/string/bundle-to-string.ts
- packages/core/src/external/fhir/export/string/resources/organization.ts
- packages/core/src/external/fhir/export/string/shared/reference.ts
- packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts
- packages/core/src/external/fhir/export/string/shared/coding.ts
- packages/core/src/external/fhir/export/string/resources/family-member-history.ts
- packages/core/src/external/fhir/export/string/shared/attachment.ts
- packages/core/src/external/fhir/export/string/resources/observation.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/fhir/codeable-concept.tspackages/core/src/external/fhir/export/string/shared/range.tspackages/core/src/external/fhir/export/string/shared/duration.tspackages/core/src/external/fhir/export/string/shared/period.tspackages/core/src/external/fhir/export/string/shared/quantity.tspackages/core/src/external/fhir/export/string/resources/medication-administration.tspackages/core/src/external/fhir/export/string/shared/identifier.tspackages/core/src/external/fhir/export/string/resources/medication-dispense.tspackages/core/src/external/fhir/export/string/shared/narrative.tspackages/core/src/external/fhir/export/string/resources/diagnostic-report.tspackages/core/src/external/fhir/export/string/shared/telecom.tspackages/core/src/external/fhir/export/string/resources/condition.tspackages/core/src/external/fhir/export/string/shared/codeable-concept.tspackages/core/src/external/fhir/export/string/shared/dosage.tspackages/core/src/external/fhir/export/string/resources/medication-request.ts
🧬 Code Graph Analysis (10)
packages/core/src/external/fhir/export/string/shared/range.ts (3)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/duration.ts (2)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/period.ts (2)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/domain/patient.ts (1)
Period(18-26)
packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)
packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/narrative.ts (2)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/telecom.ts (2)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/condition.ts (10)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(5-18)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/age.ts (1)
formatAge(8-18)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(10-33)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
packages/core/src/external/fhir/export/string/shared/dosage.ts (4)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcept(18-68)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (11)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (2)
formatIdentifiers(5-18)formatIdentifier(29-43)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
formatDosages(7-21)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/duration.ts (1)
formatDuration(8-25)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
🔇 Additional comments (25)
packages/core/src/external/fhir/export/string/shared/identifier.ts (4)
5-18: Implementation addresses previous review comments with improved APIThe
formatIdentifiersfunction now correctly handles FHIR identifiers, addressing the empty implementation issue from previous reviews. The function uses a structured object parameter approach with optional system filtering, which is more flexible than the previously suggested implementation.Good use of the
uniqfunction to remove duplicate values and proper error handling with early returns.
20-28: Well-documented JSDoc for formatIdentifierThe JSDoc comprehensively documents the purpose and parameters of the function, making it clear how the system filtering works.
29-43: Simplified implementation compared to previous suggestionThe current implementation is simpler than what was previously suggested but effectively handles the core functionality. It extracts and returns just the identifier value after validation, which is a reasonable approach if the value alone is sufficient for search purposes.
The function correctly handles:
- Undefined input
- Empty/whitespace-only values
- System filtering (when specified)
45-57: Good specialized utility function for NPI identifiersThe
formatNpiIdentifiersfunction provides a convenient wrapper for filtering NPI-specific identifiers. This specialization makes the code more readable where NPI formatting is needed and follows the single-responsibility principle.The JSDoc clearly explains the function's purpose and parameters.
packages/core/src/external/fhir/export/string/shared/telecom.ts (1)
5-29: Well-structured function with good parameter handlingThe
formatTelecomsfunction follows a consistent pattern with other formatters in the codebase:
- Object parameters with clear typing
- Good error handling with early returns
- Support for debug mode with optional labels
- Consistent use of the FIELD_SEPARATOR constant
The implementation correctly maps, filters, and joins the formatted telecom values.
packages/core/src/external/fhir/codeable-concept.ts (1)
44-46: Improved robustness with null/undefined handlingThe function now handles undefined input gracefully with an early return, making it more robust when used throughout the codebase. This is a good defensive programming practice that helps avoid potential runtime errors.
packages/core/src/external/fhir/export/string/shared/duration.ts (1)
17-25: Consider handling special cases for duration formattingThe implementation correctly handles the basic formatting of Duration objects, but it might be worth considering special handling for edge cases like zero values or unusual units. Additionally, the implementation doesn't currently handle the
systemproperty of the Duration object.However, since this appears to follow a consistent pattern with other formatters in the codebase, the implementation is acceptable as is.
packages/core/src/external/fhir/export/string/shared/period.ts (3)
11-19: Improved function signature with object destructuringThe refactored function signature using object destructuring improves readability and maintainability. This aligns well with the coding guidelines that prefer "deconstructing parameters for functions instead of multiple parameters that might be of the same type".
20-21: Good defensive programming with null checksAdding proper null checks to return
undefinedwhen the period or its properties are missing is a good practice for error handling. This ensures the function won't process invalid data.
24-24: Consistent formatting with conditional debug labelingThe conditional inclusion of the label based on the
isDebugflag provides flexibility for both debug and production use cases while maintaining a clean output format.packages/core/src/external/fhir/export/string/shared/range.ts (4)
1-5: LGTM - Clean imports and organizationThe imports are organized correctly and only include what's necessary. Good use of shared constants from other files.
10-18: Well-structured function signature with proper typingThe function signature follows the pattern established in other formatter functions, using object destructuring with appropriate typing. This is consistent with the codebase's conventions and the coding guidelines.
19-29: Good implementation of null checks and empty array handlingThe function properly handles edge cases by:
- Returning early if the range is undefined
- Only collecting valid formatted parts
- Returning undefined if no valid parts are found
This follows functional programming principles by avoiding side effects and being deterministic.
31-32: Consistent formatting with conditional debug labelingThe conditional formatting based on the
isDebugflag is consistent with other formatter functions, providing flexibility for both debug and production use cases.packages/core/src/external/fhir/export/string/shared/quantity.ts (3)
11-19: Improved function signature with object destructuringThe refactored function signature using object destructuring improves readability and maintainability, aligning with the coding guidelines that prefer "deconstructing parameters for functions instead of multiple parameters that might be of the same type".
20-21: Good use of nullish coalescing and early returnThe destructuring with nullish coalescing (
quantity ?? {}) and early return is a clean approach to handle undefined or missing values, following functional programming principles.
22-23: Clean string formatting with conditional debug labelingThe string formatting is concise and the conditional inclusion of the label based on the
isDebugflag is consistent with other formatter functions.packages/core/src/external/fhir/export/string/shared/narrative.ts (2)
19-21: Good defensive programming with null checksAdding proper null checks to return
undefinedwhen the narrative, its div property, or the filtered div is empty is a good practice for error handling.
22-28: Clean implementation using array for parts collectionUsing an array to collect parts and then joining them is a clean approach, consistent with other formatting functions. The conditional formatting based on the
isDebugflag is also consistent with the pattern used elsewhere.packages/core/src/external/fhir/export/string/resources/condition.ts (1)
69-73: Commented code clarifies search scopeThe commented-out subject reference code explicitly indicates that this property is excluded from string conversion, aligning with the PR objective to clarify which parts aren't added to the search index.
packages/core/src/external/fhir/export/string/resources/medication-request.ts (2)
76-78: Commented code clarifies search scopeThe commented-out subject reference code explicitly indicates that this property is excluded from string conversion, which aligns with the PR objective to clarify which parts of FHIR resources are not added to the search index.
138-146: Commented code clarifies search scopeThe commented-out sections for
instantiatesCanonicalandinstantiatesUriexplicitly indicate that these properties are excluded from string conversion, which aligns with the PR objective to clarify which parts of FHIR resources are not added to the search index.packages/core/src/external/fhir/export/string/shared/dosage.ts (3)
7-21: Well-structured module with consistent API designThe
formatDosagesfunction follows a consistent pattern with other formatting utilities in the codebase, supporting optional debug labeling and handling undefined/empty inputs appropriately.
23-47: Good use of object destructuring and functional compositionThe
formatDosagefunction effectively uses object destructuring for parameters and composes smaller formatting functions. The pattern of collecting components, filtering out falsy values, and joining with a separator is clean and maintainable.
49-69: Good parameter structure with defaultsThe
formatDosageDoseAndRatefunction properly uses object parameter destructuring with default values, which makes the function more maintainable and easier to extend in the future.
| .filter(Boolean); | ||
| if (performers && performers.length > 0) { | ||
| parts.push(performers.join(FIELD_SEPARATOR)); | ||
| // hasMinimumData = true; |
There was a problem hiding this comment.
Inconsistent minimum data flagging for performers.
The hasMinimumData flag assignment for performers is commented out, creating inconsistency with other entities that contribute to minimum data presence.
if (performers && performers.length > 0) {
parts.push(performers.join(FIELD_SEPARATOR));
- // hasMinimumData = true;
+ hasMinimumData = true;
}📝 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.
| // hasMinimumData = true; | |
| if (performers && performers.length > 0) { | |
| parts.push(performers.join(FIELD_SEPARATOR)); | |
| hasMinimumData = true; | |
| } |
🤖 Prompt for AI Agents
In
packages/core/src/external/fhir/export/string/resources/medication-administration.ts
at line 89, the assignment to the hasMinimumData flag for performers is
commented out, causing inconsistency with other entities that set this flag when
minimum data is present. Uncomment the line setting hasMinimumData to true for
performers to ensure consistent minimum data flagging across all relevant
entities.
There was a problem hiding this comment.
leave hasMinimumData as is for this PR
There was a problem hiding this comment.
@leite08 Understood. I'll leave the hasMinimumData flag for performers commented out as requested in this PR.
| label: "Medication", | ||
| isDebug, | ||
| }); | ||
| if (medicationRefStr) parts.push(medicationRefStr); |
There was a problem hiding this comment.
Consider setting hasMinimumData for medication reference.
While the medication codeable concept sets hasMinimumData to true when present, the medication reference doesn't, creating an inconsistency. Both types of medication identification should be treated consistently.
if (medicationRefStr) parts.push(medicationRefStr);
+ hasMinimumData = true;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/core/src/external/fhir/export/string/resources/medication-dispense.ts
at line 47, the medication reference string is pushed to parts without setting
hasMinimumData to true, unlike the medication codeable concept. Update the code
to set hasMinimumData to true when medicationRefStr is present to maintain
consistency in how medication identification is handled.
There was a problem hiding this comment.
leave hasMinimumData as is for this PR
There was a problem hiding this comment.
@leite08 Understood, I'll respect your decision to leave the hasMinimumData setting as is for this PR.
(__人__) Thanks for considering the suggestion!
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts
Show resolved
Hide resolved
packages/core/src/external/fhir/export/string/shared/codeable-concept.ts
Outdated
Show resolved
Hide resolved
packages/core/src/external/fhir/export/string/resources/medication-request.ts
Show resolved
Hide resolved
5fb1757 to
76fe889
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (1)
28-31: Missing hasMinimumData update for status field.Unlike other fields like
codeandconclusion, the status field does not contribute tohasMinimumData. For consistency, consider whether status should count toward minimum data presence.if (report.status) { const status = emptyIfDenied(report.status); if (status) parts.push(isDebug ? `Status: ${status}` : status); + hasMinimumData = true; }packages/core/src/external/fhir/export/string/resources/medication-request.ts (2)
69-74:hasMinimumDatais not updated whenmedicationReferenceis presentThe code sets
hasMinimumData = truewhenmedicationCodeableConceptis formatted (lines 64-67), but not when onlymedicationReferenceis present. If a resource contains only a medication reference and none of the other minimum-data fields, it will incorrectly returnundefined.Apply this fix:
-if (medicationRefStr) parts.push(medicationRefStr); +if (medicationRefStr) { + parts.push(medicationRefStr); + hasMinimumData = true; +}
93-94: Raw date strings require formattingSimilar to past review feedback, raw ISO date strings are being output without formatting, which reduces readability.
Apply this fix:
-if (request.authoredOn) - parts.push(isDebug ? `Authored On: ${request.authoredOn}` : request.authoredOn); +if (request.authoredOn) { + const formattedDate = buildDayjs(request.authoredOn).format("YYYY-MM-DD"); + parts.push(isDebug ? `Authored On: ${formattedDate}` : formattedDate); +}Remember to import
buildDayjs:+import { buildDayjs } from "../../../../shared/date";packages/core/src/external/fhir/export/string/resources/condition.ts (2)
18-19: Re-evaluate the "minimum data" ruleThe
hasMinimumDatainitialization and validation logic only considers a condition valid whencondition.codeis present. This restrictive approach could exclude legitimate clinical statements that might lack a code but contain other meaningful data (e.g., conditions with only category or severity specified due to privacy reasons or SDOH conditions).Consider broadening the criteria by including other relevant fields or delegating the minimum data validation to the caller instead of hard-coding it in this class.
Also applies to: 57-60, 187-188
82-84: Raw date/time values are output without formattingThe datetime fields (
onsetDateTime,abatementDateTime, andrecordedDate) are output as raw ISO strings without proper formatting, which reduces readability and consistency with other formatted dates in the application.-if (condition.onsetDateTime) { - parts.push(isDebug ? `Onset Date: ${condition.onsetDateTime}` : condition.onsetDateTime); +if (condition.onsetDateTime) { + const formattedDate = buildDayjs(condition.onsetDateTime).format("YYYY-MM-DD"); + parts.push(isDebug ? `Onset Date: ${formattedDate}` : formattedDate); }Apply similar changes to
abatementDateTimeandrecordedDateas well.Also applies to: 103-107, 132-134
🧹 Nitpick comments (5)
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (1)
20-21: Consider including isDebug parameter in formatIdentifiers call.Unlike other formatting calls in the file that pass the
isDebugparameter, this call doesn't include it. For consistency with the pattern used throughout the file, consider updating this call.- const identifierStr = formatIdentifiers({ identifiers: report.identifier }); + const identifierStr = formatIdentifiers({ identifiers: report.identifier, isDebug });packages/core/src/external/fhir/export/string/resources/medication-request.ts (2)
138-146: Consider using consistent commenting styleThese commented-out blocks use a different style than the commented-out lines at 76-77, making the code less consistent.
-// if (request.instantiatesCanonical) { -// const instCanonical = request.instantiatesCanonical.join(FIELD_SEPARATOR); -// parts.push(isDebug ? `Instantiates Canonical: ${instCanonical}` : instCanonical); -// } - -// if (request.instantiatesUri) { -// const instUri = request.instantiatesUri.join(FIELD_SEPARATOR); -// parts.push(isDebug ? `Instantiates URI: ${instUri}` : instUri); -// } +/* const instantiatesCanonicalStr = request.instantiatesCanonical?.join(FIELD_SEPARATOR); +if (instantiatesCanonicalStr) { + parts.push(isDebug ? `Instantiates Canonical: ${instantiatesCanonicalStr}` : instantiatesCanonicalStr); +} + +const instantiatesUriStr = request.instantiatesUri?.join(FIELD_SEPARATOR); +if (instantiatesUriStr) { + parts.push(isDebug ? `Instantiates URI: ${instantiatesUriStr}` : instantiatesUriStr); +} */
187-201: Use consistent variable naming patternsNaming patterns vary between sections - here
initialFillPartsis used while elsewhere you usedispensePartsandsubstitutionParts. This inconsistency makes the code harder to follow.- const initialFillParts = []; + const fillParts = []; ... - if (quantityStr) initialFillParts.push(quantityStr); + if (quantityStr) fillParts.push(quantityStr); ... - if (durationStr) initialFillParts.push(isDebug ? `Duration: ${durationStr}` : durationStr); + if (durationStr) fillParts.push(isDebug ? `Duration: ${durationStr}` : durationStr); ... - if (initialFillParts.length > 0) { - const initialFillStr = initialFillParts.join(FIELD_SEPARATOR); + if (fillParts.length > 0) { + const initialFillStr = fillParts.join(FIELD_SEPARATOR);packages/core/src/external/fhir/export/string/resources/condition.ts (2)
69-73: Document the reason for commented-out codeThere is commented-out code for subject reference formatting. Either remove it if it's no longer needed or add a comment explaining why it's been excluded from the string conversion.
-// const subjectStr = formatReference(condition.subject, "Subject"); -// if (subjectStr) { -// parts.push(subjectStr); -// hasMinimumData = true; -// } +// Subject is intentionally excluded from string conversion for the following reason: +// [Add explanation here about why subject is excluded from search indexing]
160-163: Simplify array handling logicThe checks for
stages && stages.length > 0andevidence && evidence.length > 0are redundant since.filter(Boolean)already handles empty arrays in the previous lines.-if (stages && stages.length > 0) { +if (stages.length > 0) { const stagesStr = stages.join(FIELD_SEPARATOR); parts.push(isDebug ? `Stages: ${stagesStr}` : stagesStr); }And similarly for the evidence section:
-if (evidence && evidence.length > 0) { +if (evidence.length > 0) { const evidenceStr = evidence.join(FIELD_SEPARATOR); parts.push(isDebug ? `Evidence: ${evidenceStr}` : evidenceStr); }Also applies to: 174-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (46)
packages/core/src/external/fhir/codeable-concept.ts(1 hunks)packages/core/src/external/fhir/export/string/bundle-to-string.ts(6 hunks)packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/communication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/composition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/condition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/consent.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/coverage.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/document-reference.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/encounter.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/family-member-history.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/immunization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/location.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-administration.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-request.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-statement.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/observation.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/organization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/patient.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/practitioner.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/procedure.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/related-person.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts(6 hunks)packages/core/src/external/fhir/export/string/shared/address.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/age.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/annotation.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/attachment.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/code-display.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts(2 hunks)packages/core/src/external/fhir/export/string/shared/coding.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/debug.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/human-name.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/identifier.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/narrative.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/period.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/quantity.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/range.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/telecom.ts(1 hunks)packages/core/src/external/fhir/normalization/resources/encounter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/external/fhir/export/string/shared/attachment.ts
🚧 Files skipped from review as they are similar to previous changes (42)
- packages/core/src/external/fhir/export/string/shared/debug.ts
- packages/core/src/external/fhir/normalization/resources/encounter.ts
- packages/core/src/external/fhir/codeable-concept.ts
- packages/core/src/external/fhir/export/string/shared/tests/codeable-concept.test.ts
- packages/core/src/external/fhir/export/string/shared/code-display.ts
- packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts
- packages/core/src/external/fhir/export/string/shared/age.ts
- packages/core/src/external/fhir/export/string/shared/duration.ts
- packages/core/src/external/fhir/export/string/resources/practitioner.ts
- packages/core/src/external/fhir/export/string/resources/composition.ts
- packages/core/src/external/fhir/export/string/resources/related-person.ts
- packages/core/src/external/fhir/export/string/resources/coverage.ts
- packages/core/src/external/fhir/export/string/resources/location.ts
- packages/core/src/external/fhir/export/string/resources/patient.ts
- packages/core/src/external/fhir/export/string/resources/communication.ts
- packages/core/src/external/fhir/export/string/shared/quantity.ts
- packages/core/src/external/fhir/export/string/shared/period.ts
- packages/core/src/external/fhir/export/string/resources/immunization.ts
- packages/core/src/external/fhir/export/string/resources/medication-statement.ts
- packages/core/src/external/fhir/export/string/resources/medication.ts
- packages/core/src/external/fhir/export/string/resources/procedure.ts
- packages/core/src/external/fhir/export/string/shared/identifier.ts
- packages/core/src/external/fhir/export/string/resources/consent.ts
- packages/core/src/external/fhir/export/string/resources/document-reference.ts
- packages/core/src/external/fhir/export/string/resources/medication-dispense.ts
- packages/core/src/external/fhir/export/string/shared/narrative.ts
- packages/core/src/external/fhir/export/string/resources/medication-administration.ts
- packages/core/src/external/fhir/export/string/shared/address.ts
- packages/core/src/external/fhir/export/string/shared/reference.ts
- packages/core/src/external/fhir/export/string/shared/range.ts
- packages/core/src/external/fhir/export/string/resources/encounter.ts
- packages/core/src/external/fhir/export/string/resources/family-member-history.ts
- packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts
- packages/core/src/external/fhir/export/string/bundle-to-string.ts
- packages/core/src/external/fhir/export/string/shared/annotation.ts
- packages/core/src/external/fhir/export/string/shared/human-name.ts
- packages/core/src/external/fhir/export/string/shared/dosage.ts
- packages/core/src/external/fhir/export/string/resources/organization.ts
- packages/core/src/external/fhir/export/string/resources/observation.ts
- packages/core/src/external/fhir/export/string/shared/codeable-concept.ts
- packages/core/src/external/fhir/export/string/shared/telecom.ts
- packages/core/src/external/fhir/export/string/shared/coding.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/fhir/export/string/resources/condition.tspackages/core/src/external/fhir/export/string/resources/diagnostic-report.tspackages/core/src/external/fhir/export/string/resources/medication-request.ts
🧬 Code Graph Analysis (2)
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (8)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(5-18)packages/core/src/external/fhir/export/string/shared/deny.ts (1)
emptyIfDenied(9-15)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(8-26)packages/core/src/external/fhir/resources/diagnostic-report.ts (1)
presentedFormsToText(6-14)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (11)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (2)
formatIdentifiers(5-18)formatIdentifier(29-43)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
formatDosages(7-21)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/duration.ts (1)
formatDuration(8-25)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
🔇 Additional comments (19)
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (14)
3-9: Updated imports to support new functionality.The imports have been properly updated to use the modular formatting utilities and the new
defaultHasMinimumDataconstant, which aligns with the broader refactoring strategy across FHIR resource converters.
16-17: Method signature enhancement aligns with the standardized approach.The addition of the optional
isDebugparameter improves flexibility by allowing control over output format, and the use ofdefaultHasMinimumDataensures consistency with other resource converters.
23-26: Explicitly commented-out code clarifies excluded properties.These commented-out sections clearly indicate which properties are intentionally excluded from string conversion, matching the PR objective of defining the scope of searchable content.
33-38: Improved formatting with object parameter syntax.The code has been refactored to use object parameter syntax with named properties, which improves readability and maintainability. The inclusion of the
isDebugparameter ensures consistent behavior with other formatting utilities.
40-46: Code field properly updates hasMinimumData flag.The code correctly sets
hasMinimumData = truewhen a valid code is present, ensuring that important diagnostic information contributes to the data presence check.
61-72: Enhanced date and period formatting with improved labels.The effectiveDateTime now uses a more descriptive label ("Effective Date") in debug mode, and the effectivePeriod formatting has been updated to use the modular
formatPeriodfunction with appropriate parameters.
76-82: Added support for imagingStudy references with debug-aware labels.The implementation properly formats imaging study references using the updated
formatReferencesutility with appropriate labeling based on the debug flag.
99-119: Added comprehensive support for additional references.The code now properly formats performer, interpreter, and specimen references with appropriate debug-aware labels, enhancing the completeness of the string representation.
124-126: Presented forms correctly update hasMinimumData flag.The code properly sets
hasMinimumData = truewhen presented forms are available, ensuring that this important diagnostic information contributes to the data presence check.
129-132: Conclusion field properly updates hasMinimumData flag.The code correctly sets
hasMinimumData = truewhen a conclusion is present, ensuring that this important diagnostic information contributes to the data presence check.
134-141: Added support for conclusionCode with appropriate formatting.The implementation properly formats conclusion codes using the updated
formatCodeableConceptsutility with appropriate labeling based on the debug flag.
143-144: Added support for narrative text with appropriate formatting.The implementation properly formats the narrative text using the new
formatNarrativeutility with appropriate labeling based on the debug flag.
146-151: Enhanced result references formatting with debug-aware labels.The implementation properly formats result references using the updated
formatReferencesutility with appropriate labeling based on the debug flag.
153-153: Improved minimum data check for final output.The final return condition properly uses the
hasMinimumDataflag to determine if there's enough meaningful data to generate a string representation.packages/core/src/external/fhir/export/string/resources/medication-request.ts (4)
18-19: LGTM: Minimum data checking is properly implementedThe implementation of
hasMinimumDatawith initialization todefaultHasMinimumDataand returningundefinedwhen minimum data is absent is a good pattern that ensures only meaningful resources are converted to strings.Also applies to: 304-306
2-12: Good modularization of formatting utilitiesThe refactoring to import shared formatting utilities enhances maintainability and consistency across FHIR resource string converters.
25-33: LGTM: Consistent use of debug labels across fieldsThe implementation consistently applies conditional debug labels to all fields, enabling clear and detailed output during debugging while maintaining clean output for production uses.
Also applies to: 34-46, 48-57
175-180: Excellent structuring of complex nested objectsThe implementation does a great job organizing the complex parts of MedicationRequest (dosage instructions, dispense request, substitution) into well-structured sections with appropriate minimum data checks.
Also applies to: 182-252, 254-278
packages/core/src/external/fhir/export/string/resources/condition.ts (1)
17-187:❌ Incorrect review comment
Consider adding error handling to the toString method
The method lacks error handling for potential exceptions that could be thrown by formatting functions. If any formatter throws an error, it will propagate to the caller, potentially causing service disruptions.
Consider wrapping the entire method body in a try-catch block and returning undefined or a fallback string in case of errors:
toString(condition: Condition, isDebug?: boolean): string | undefined { + try { let hasMinimumData = defaultHasMinimumData; const parts: string[] = []; // ... rest of the method ... if (!hasMinimumData) return undefined; return parts.join(FIELD_SEPARATOR); + } catch (error) { + // Log the error or handle it appropriately + return undefined; // Or a fallback string + } }
🌐 Web query:
Is error handling commonly implemented in toString methods in TypeScript?💡 Result:
Error handling is not commonly implemented directly in
toString()methods in TypeScript, but there are important considerations when working with this method to avoid errors.Standard toString() Implementation
The
toString()method is primarily designed to return a string representation of an object rather than handle errors. In JavaScript and TypeScript, the default implementation oftoString()comes fromObject.prototype.toString(), which returns"[object Object]"for most objects[4]. Error objects have their own implementation that returns a formatted string with the error name and message[1].The standard
Error.prototype.toString()implementation follows this pattern:Error.prototype.toString = function () { if (this === null || (typeof this !== "object" && typeof this !== "function")) { throw new TypeError(); } let name = this.name; name = name === undefined ? "Error" : `${name}`; let msg = this.message; msg = msg === undefined ? "" : `${msg}`; if (name === "") { return msg; } if (msg === "") { return name; } return `${name}: ${msg}`; };As shown, the built-in implementation does include basic error checking by throwing a TypeError if the method is called with an invalid context[1].
Error Potential When Using toString()
The main issue with
toString()is not within its implementation but when calling it on potentially null or undefined values. This is a common pitfall:// This will throw an error if variable is null or undefined variable.toString();This issue has been documented as a common source of runtime errors in JavaScript applications[2]. The safer approach is to use type checking before calling the method or using alternative string conversion techniques:
// Safer approach String(variable); // Works with null and undefinedCustom Error Handling Utilities
For more robust error handling, developers often create custom utilities rather than modifying
toString()methods. For example, a custom error printing utility can be implemented to handle error chains with causes:function printError(error: Error): string { let maybeCause = error.cause instanceof Error ? printError(error.cause) : error.cause ? error.cause.toString() : null; let cause = maybeCause ? `\n\tcaused by: ${maybeCause}` : ''; return `${error.name}: ${error.message}${cause}`; }This approach provides more structured error information than a standard
toString()call would[3].Custom Classes with Enhanced toString()
When creating custom classes, developers can override the
toString()method to provide more meaningful string representations:class Dog { constructor(name, breed, color, sex) { this.name = name; this.breed = breed; this.color = color; this.sex = sex; } toString() { return `Dog ${this.name} is a ${this.sex} ${this.color} ${this.breed}`; } }While this example doesn't specifically show error handling[4], custom classes can include error handling in their
toString()methods if necessary.Best Practices
When working with error handling in TypeScript, the focus is typically on:
- Using TypeScript's type system to catch errors early[7]
- Implementing proper error classes and error checking[8]
- Using result types or union types to represent success and failure states[7]
- Type guards to safely handle errors[8]
Rather than putting error handling in
toString()methods, the recommendation is to structure your application with proper error handling patterns and usetoString()primarily for its intended purpose of providing string representations.Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/toString
- 2: https://dev.to/manikbajaj/avoid-tostring-fatal-error-string-casting-in-javascript-2mn6
- 3: https://v5.chriskrycho.com/notes/a-reasonable-error-printing-utility-in-typescript/
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/toString
- 5: https://www.tutorialspoint.com/error-prototype-tostring-method-in-javascript
- 6: https://www.zipy.ai/blog/typescript-errors
- 7: https://claritydev.net/blog/typescript-error-handling-and-defensive-programming
- 8: https://www.dhiwise.com/post/typescript-error-handling-pitfalls-and-how-to-avoid-them
- 9: https://engineering.udacity.com/handling-errors-like-a-pro-in-typescript-d7a314ad4991
- 10: https://plain.com/blog/error-handling-in-typescript-like-a-pro
🏁 Script executed:
#!/bin/bash # Search for formatCodeableConcept implementation rg "function formatCodeableConcept" -A 10 -B 3 --no-heading rg "function formatCodeableConcepts" -A 10 -B 3 --no-heading rg "function formatReference" -A 10 -B 3 --no-heading rg "function formatRange" -A 10 -B 3 --no-headingLength of output: 8224
Ignore adding a try-catch around
toStringThe various
format*helpers guard against missing or undefined inputs and returnundefinedrather than throwing. It’s more idiomatic to keeptoStringfocused on producing a string representation—any unexpected errors should be caught and handled at a higher level, not swallowed silently here.Likely an incorrect or invalid review comment.
packages/core/src/external/fhir/export/string/resources/medication-request.ts
Show resolved
Hide resolved
2ab4ea0 to
5ff247e
Compare
15a09d4 to
fcbea8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/core/src/external/fhir/export/string/resources/condition.ts (5)
1-11: 🛠️ Refactor suggestionImport the buildDayjs utility for date formatting
The file is missing the import for the
buildDayjsutility function, which is needed to properly format date/time fields throughout the code.import { Condition, ConditionEvidence, ConditionStage } from "@medplum/fhirtypes"; import { defaultHasMinimumData, FHIRResourceToString } from "../fhir-resource-to-string"; import { formatAge } from "../shared/age"; import { formatAnnotations } from "../shared/annotation"; import { formatCodeableConcept, formatCodeableConcepts } from "../shared/codeable-concept"; import { formatIdentifiers } from "../shared/identifier"; import { formatNarrative } from "../shared/narrative"; import { formatPeriod } from "../shared/period"; import { formatRange } from "../shared/range"; import { formatReference, formatReferences } from "../shared/reference"; import { FIELD_SEPARATOR } from "../shared/separator"; +import { buildDayjs } from "../../../../../../shared/date-time";
18-19: 🛠️ Refactor suggestionRe-evaluate the "minimum data" rule
The
hasMinimumDataflag is set to true only ifcondition.codeis present. This approach is too restrictive and might exclude valid clinical statements that don't have a code but have other meaningful data like category or severity.Consider broadening the criteria to include other fields or delegating the minimum data decision to the caller. For example:
- let hasMinimumData = defaultHasMinimumData; + let hasMinimumData = defaultHasMinimumData || + !!condition.category?.length || + !!condition.severity;
82-84: 🛠️ Refactor suggestionFormat date/time values for better readability
Raw ISO date strings are being output directly without formatting, which reduces readability and locale consistency.
- if (condition.onsetDateTime) { - parts.push(isDebug ? `Onset Date: ${condition.onsetDateTime}` : condition.onsetDateTime); + if (condition.onsetDateTime) { + const formatted = buildDayjs(condition.onsetDateTime).format("YYYY-MM-DD"); + parts.push(isDebug ? `Onset Date: ${formatted}` : formatted); }
103-107: 🛠️ Refactor suggestionFormat date/time values for better readability
Raw ISO date strings are being output directly without formatting, which reduces readability and locale consistency.
- if (condition.abatementDateTime) { - parts.push( - isDebug ? `Abatement Date: ${condition.abatementDateTime}` : condition.abatementDateTime - ); + if (condition.abatementDateTime) { + const formatted = buildDayjs(condition.abatementDateTime).format("YYYY-MM-DD"); + parts.push( + isDebug ? `Abatement Date: ${formatted}` : formatted + ); }
132-134: 🛠️ Refactor suggestionFormat date/time values for better readability
Raw ISO date strings are being output directly without formatting, which reduces readability and locale consistency.
- if (condition.recordedDate) { - parts.push(isDebug ? `Recorded Date: ${condition.recordedDate}` : condition.recordedDate); + if (condition.recordedDate) { + const formatted = buildDayjs(condition.recordedDate).format("YYYY-MM-DD"); + parts.push(isDebug ? `Recorded Date: ${formatted}` : formatted); }
🧹 Nitpick comments (4)
packages/core/src/external/fhir/export/string/resources/condition.ts (4)
150-163: Consider creating a reusable helper for complex nested objectsThe mapping of stage array elements to strings could be extracted into a dedicated helper function to improve code organization and reusability, especially since you have a similar pattern with evidence.
+ private formatStage(stage: ConditionStage, isDebug?: boolean): string | undefined { + const components = [ + formatCodeableConcept({ concept: stage.summary, label: "Summary", isDebug }), + formatReferences({ references: stage.assessment, label: "Assessment", isDebug }), + formatCodeableConcept({ concept: stage.type, label: "Type", isDebug }), + ].filter(Boolean); + return components.length > 0 ? components.join(FIELD_SEPARATOR) : undefined; + } toString(condition: Condition, isDebug?: boolean): string | undefined { // other code... - const stages = condition.stage - ?.map((stage: ConditionStage) => { - const components = [ - formatCodeableConcept({ concept: stage.summary, label: "Summary", isDebug }), - formatReferences({ references: stage.assessment, label: "Assessment", isDebug }), - formatCodeableConcept({ concept: stage.type, label: "Type", isDebug }), - ].filter(Boolean); - return components.join(FIELD_SEPARATOR); - }) - .filter(Boolean); - if (stages && stages.length > 0) { - const stagesStr = stages.join(FIELD_SEPARATOR); - parts.push(isDebug ? `Stages: ${stagesStr}` : stagesStr); + const stages = condition.stage + ?.map(stage => this.formatStage(stage, isDebug)) + .filter(Boolean); + if (stages?.length > 0) { + const stagesStr = stages.join(FIELD_SEPARATOR); + parts.push(isDebug ? `Stages: ${stagesStr}` : stagesStr); }
165-177: Consider creating a reusable helper for complex nested objectsSimilar to the suggestion for stages, the mapping of evidence array elements could be extracted into a dedicated helper function.
+ private formatEvidence(evidence: ConditionEvidence, isDebug?: boolean): string | undefined { + const components = [ + formatCodeableConcepts({ concepts: evidence.code, label: "Code", isDebug }), + formatReferences({ references: evidence.detail, label: "Detail", isDebug }), + ].filter(Boolean); + return components.length > 0 ? components.join(FIELD_SEPARATOR) : undefined; + } toString(condition: Condition, isDebug?: boolean): string | undefined { // other code... - const evidence = condition.evidence - ?.map((ev: ConditionEvidence) => { - const components = [ - formatCodeableConcepts({ concepts: ev.code, label: "Code", isDebug }), - formatReferences({ references: ev.detail, label: "Detail", isDebug }), - ].filter(Boolean); - return components.join(FIELD_SEPARATOR); - }) - .filter(Boolean); - if (evidence && evidence.length > 0) { - const evidenceStr = evidence.join(FIELD_SEPARATOR); - parts.push(isDebug ? `Evidence: ${evidenceStr}` : evidenceStr); + const evidence = condition.evidence + ?.map(ev => this.formatEvidence(ev, isDebug)) + .filter(Boolean); + if (evidence?.length > 0) { + const evidenceStr = evidence.join(FIELD_SEPARATOR); + parts.push(isDebug ? `Evidence: ${evidenceStr}` : evidenceStr); }
86-88: Consider consistent formatting for age valuesThe formatting of onset and abatement ages differs slightly. For onset age, you directly use the formatted string, but for abatement age, you wrap it in an additional conditional. Both approaches achieve the same result, but consistency would improve code maintainability.
const ageStr = formatAge({ age: condition.onsetAge, isDebug }); - if (ageStr) parts.push(isDebug ? `Onset Age: ${ageStr}` : ageStr); + if (ageStr) { + parts.push(isDebug ? `Onset Age: ${ageStr}` : ageStr); + }Or the reverse:
const abatementAgeStr = formatAge({ age: condition.abatementAge, isDebug }); - if (abatementAgeStr) { - parts.push(isDebug ? `Abatement Age: ${abatementAgeStr}` : abatementAgeStr); - } + if (abatementAgeStr) parts.push(isDebug ? `Abatement Age: ${abatementAgeStr}` : abatementAgeStr);Also applies to: 109-112
82-134: Consider creating a helper function for date formattingThere's repetitive code for handling different date fields (onset, abatement, recorded). This could be extracted into a helper function to reduce duplication.
+ private formatDateField({ + dateValue, + label, + isDebug, + }: { + dateValue: string | undefined; + label: string; + isDebug?: boolean; + }): string | undefined { + if (!dateValue) return undefined; + const formatted = buildDayjs(dateValue).format("YYYY-MM-DD"); + return isDebug ? `${label}: ${formatted}` : formatted; + } toString(condition: Condition, isDebug?: boolean): string | undefined { // other code... - if (condition.onsetDateTime) { - parts.push(isDebug ? `Onset Date: ${condition.onsetDateTime}` : condition.onsetDateTime); + const onsetDateStr = this.formatDateField({ + dateValue: condition.onsetDateTime, + label: "Onset Date", + isDebug, + }); + if (onsetDateStr) { + parts.push(onsetDateStr); } // similar changes for abatementDateTime and recordedDate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (47)
packages/core/src/external/fhir/codeable-concept.ts(1 hunks)packages/core/src/external/fhir/export/string/bundle-to-string.ts(6 hunks)packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/communication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/composition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/condition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/consent.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/coverage.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/document-reference.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/encounter.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/family-member-history.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/immunization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/location.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-administration.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-request.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-statement.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/observation.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/organization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/patient.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/practitioner.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/procedure.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/related-person.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts(6 hunks)packages/core/src/external/fhir/export/string/shared/address.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/age.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/annotation.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/attachment.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/code-display.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts(2 hunks)packages/core/src/external/fhir/export/string/shared/coding.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/debug.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/deny.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/human-name.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/identifier.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/narrative.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/period.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/quantity.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/range.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/telecom.ts(1 hunks)packages/core/src/external/fhir/normalization/resources/encounter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/external/fhir/export/string/shared/annotation.ts
🚧 Files skipped from review as they are similar to previous changes (45)
- packages/core/src/external/fhir/normalization/resources/encounter.ts
- packages/core/src/external/fhir/codeable-concept.ts
- packages/core/src/external/fhir/export/string/shared/debug.ts
- packages/core/src/external/fhir/export/string/shared/age.ts
- packages/core/src/external/fhir/export/string/shared/tests/codeable-concept.test.ts
- packages/core/src/external/fhir/export/string/shared/code-display.ts
- packages/core/src/external/fhir/export/string/shared/deny.ts
- packages/core/src/external/fhir/export/string/resources/practitioner.ts
- packages/core/src/external/fhir/export/string/resources/consent.ts
- packages/core/src/external/fhir/export/string/resources/composition.ts
- packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts
- packages/core/src/external/fhir/export/string/resources/location.ts
- packages/core/src/external/fhir/export/string/shared/quantity.ts
- packages/core/src/external/fhir/export/string/shared/duration.ts
- packages/core/src/external/fhir/export/string/resources/medication.ts
- packages/core/src/external/fhir/export/string/resources/coverage.ts
- packages/core/src/external/fhir/export/string/resources/communication.ts
- packages/core/src/external/fhir/export/string/shared/period.ts
- packages/core/src/external/fhir/export/string/resources/document-reference.ts
- packages/core/src/external/fhir/export/string/shared/identifier.ts
- packages/core/src/external/fhir/export/string/resources/patient.ts
- packages/core/src/external/fhir/export/string/resources/related-person.ts
- packages/core/src/external/fhir/export/string/shared/narrative.ts
- packages/core/src/external/fhir/export/string/resources/organization.ts
- packages/core/src/external/fhir/export/string/shared/range.ts
- packages/core/src/external/fhir/export/string/resources/procedure.ts
- packages/core/src/external/fhir/export/string/resources/family-member-history.ts
- packages/core/src/external/fhir/export/string/shared/attachment.ts
- packages/core/src/external/fhir/export/string/bundle-to-string.ts
- packages/core/src/external/fhir/export/string/resources/medication-dispense.ts
- packages/core/src/external/fhir/export/string/resources/medication-statement.ts
- packages/core/src/external/fhir/export/string/resources/observation.ts
- packages/core/src/external/fhir/export/string/shared/reference.ts
- packages/core/src/external/fhir/export/string/shared/address.ts
- packages/core/src/external/fhir/export/string/shared/human-name.ts
- packages/core/src/external/fhir/export/string/shared/codeable-concept.ts
- packages/core/src/external/fhir/export/string/resources/medication-administration.ts
- packages/core/src/external/fhir/export/string/resources/immunization.ts
- packages/core/src/external/fhir/export/string/shared/telecom.ts
- packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts
- packages/core/src/external/fhir/export/string/resources/encounter.ts
- packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts
- packages/core/src/external/fhir/export/string/shared/dosage.ts
- packages/core/src/external/fhir/export/string/shared/coding.ts
- packages/core/src/external/fhir/export/string/resources/medication-request.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/fhir/export/string/resources/condition.ts
🧬 Code Graph Analysis (1)
packages/core/src/external/fhir/export/string/resources/condition.ts (10)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(5-18)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/age.ts (1)
formatAge(8-18)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(10-33)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
5ff247e to
9caf6fb
Compare
fcbea8b to
26fe802
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
packages/core/src/external/fhir/export/string/resources/condition.ts (5)
1-11: Import buildDayjs utility to properly format datesThe code should use the
buildDayjsutility to format date/time fields for better readability, but this utility is not imported despite being mentioned in previous review comments.import { Condition, ConditionEvidence, ConditionStage } from "@medplum/fhirtypes"; import { defaultHasMinimumData, FHIRResourceToString } from "../fhir-resource-to-string"; import { formatAge } from "../shared/age"; import { formatAnnotations } from "../shared/annotation"; import { formatCodeableConcept, formatCodeableConcepts } from "../shared/codeable-concept"; import { formatIdentifiers } from "../shared/identifier"; import { formatNarrative } from "../shared/narrative"; import { formatPeriod } from "../shared/period"; import { formatRange } from "../shared/range"; import { formatReference, formatReferences } from "../shared/reference"; import { FIELD_SEPARATOR } from "../shared/separator"; +import { buildDayjs } from "../../../../../../shared/date-time";
17-18: Re-evaluate minimum data validation criteriaThe minimum data validation only checks for the presence of
condition.code, which may exclude valid clinical statements missing this field but containing other relevant information.Consider broadening the criteria or delegating the validation to the caller instead of hard-coding it here. Valid conditions might have meaningful data in
categoryorseverityeven without a code.
82-84: Format date/time fields for better readabilityRaw ISO date strings are being output without formatting, which reduces readability and locale consistency.
-if (condition.onsetDateTime) { - parts.push(isDebug ? `Onset Date: ${condition.onsetDateTime}` : condition.onsetDateTime); +if (condition.onsetDateTime) { + const formattedDate = buildDayjs(condition.onsetDateTime).format("YYYY-MM-DD"); + parts.push(isDebug ? `Onset Date: ${formattedDate}` : formattedDate); }
103-107: Format abatement date for better readabilitySimilar to the onset date, the abatement date should be formatted for improved readability.
-if (condition.abatementDateTime) { - parts.push( - isDebug ? `Abatement Date: ${condition.abatementDateTime}` : condition.abatementDateTime - ); +if (condition.abatementDateTime) { + const formattedDate = buildDayjs(condition.abatementDateTime).format("YYYY-MM-DD"); + parts.push(isDebug ? `Abatement Date: ${formattedDate}` : formattedDate); }
132-134: Format recorded date for better readabilityThe recorded date should also use the date formatting utility.
-if (condition.recordedDate) { - parts.push(isDebug ? `Recorded Date: ${condition.recordedDate}` : condition.recordedDate); +if (condition.recordedDate) { + const formattedDate = buildDayjs(condition.recordedDate).format("YYYY-MM-DD"); + parts.push(isDebug ? `Recorded Date: ${formattedDate}` : formattedDate); }packages/core/src/external/fhir/export/string/resources/medication-request.ts (3)
69-74:⚠️ Potential issueMissing check to update
hasMinimumDatafor medicationReferenceWhen
medicationReferenceis present, the code adds it topartsbut doesn't updatehasMinimumData, unlike whenmedicationCodeableConceptis present. This could cause valid MedicationRequest resources that only identify the medication via reference to be incorrectly excluded from search results.const medicationRefStr = formatReference({ reference: request.medicationReference, label: "Medication", isDebug, }); -if (medicationRefStr) parts.push(medicationRefStr); +if (medicationRefStr) { + parts.push(medicationRefStr); + hasMinimumData = true; +}
93-94: 🛠️ Refactor suggestionRaw date strings require formatting
The
authoredOndate is being output as a raw ISO string, which reduces readability.+import { buildDayjs } from "../../../../shared/date"; // Then replace lines 93-94 with: -if (request.authoredOn) - parts.push(isDebug ? `Authored On: ${request.authoredOn}` : request.authoredOn); +if (request.authoredOn) { + const formattedDate = buildDayjs(request.authoredOn).format("YYYY-MM-DD"); + parts.push(isDebug ? `Authored On: ${formattedDate}` : formattedDate); +}
240-246: 🛠️ Refactor suggestionPrevent duplicate performer entries in dispenseRequest
The code currently formats both
request.performerandrequest.dispenseRequest.performerindependently, which will produce duplicate entries if they reference the same practitioner/organization.-const performerStr = formatReference({ - reference: dispense.performer, - label: "Dispense Performer", - isDebug, -}); -if (performerStr) dispenseParts.push(performerStr); +// Only add dispense.performer if it's different from request.performer +const requestPerformerRef = request.performer?.reference; +const dispensePerformerRef = dispense.performer?.reference; +if (dispensePerformerRef && dispensePerformerRef !== requestPerformerRef) { + const performerStr = formatReference({ + reference: dispense.performer, + label: "Dispense Performer", + isDebug, + }); + if (performerStr) dispenseParts.push(performerStr); +}
🧹 Nitpick comments (9)
packages/core/src/external/fhir/export/string/resources/condition.ts (1)
187-189: Consider adding logging for debug purposesWhen returning
undefineddue to missing minimum data, consider adding a debug log to help track instances where conditions are being filtered out. This would be valuable for diagnosing unexpected search behavior.-if (!hasMinimumData) return undefined; +if (!hasMinimumData) { + // Using console.log is acceptable here as it's only for debug purposes + if (isDebug) console.log(`Condition ${condition.id} skipped due to missing minimum data`); + return undefined; +}packages/core/src/external/fhir/export/string/resources/consent.ts (5)
40-43: Remove commented code or add explanationCommented-out code without explanation of why it's commented reduces code readability. Either:
- Remove it completely if it's not needed
- Add a comment explaining why it's preserved but commented out
- Implement it if it should be included
The coding guidelines mention that comments should explain "why" not "how".
- // if (consent.patient) { - // const patientStr = formatReferences([consent.patient], "Patient"); - // if (patientStr) parts.push(patientStr); - // } + // Patient reference is intentionally excluded from search index + // because [ADD EXPLANATION HERE]
61-69: Consider consistent pattern for adding debug labelsAt line 67, there's a redundant debug label since
formatAttachmentalready handles the debug label. This creates inconsistency with other formatting patterns in this file.- parts.push(isDebug ? `Source: ${attachment}` : attachment); + parts.push(attachment);
122-211: Consider extracting provision formatting to a separate methodThe provision formatting logic (lines 122-211) is complex and makes the
toStringmethod quite long. Consider extracting this to a separate private method for better readability and maintainability.toString(consent: Consent, isDebug?: boolean): string | undefined { let hasMinimumData = defaultHasMinimumData; const parts: string[] = []; // ... existing code ... if (consent.provision) { - const provision = consent.provision; - const provisionParts = []; - - if (provision.type) provisionParts.push(isDebug ? `Type: ${provision.type}` : provision.type); - // ... rest of provision formatting ... + const provisionResult = this.formatProvision(consent.provision, isDebug); + if (provisionResult) { + parts.push(provisionResult.formattedString); + hasMinimumData = provisionResult.hasData || hasMinimumData; + } } // ... rest of the method ... } private formatProvision(provision: ConsentProvision, isDebug?: boolean): { formattedString: string; hasData: boolean } | undefined { const provisionParts = []; let hasData = false; if (provision.type) provisionParts.push(isDebug ? `Type: ${provision.type}` : provision.type); // ... rest of provision formatting logic ... if (provisionParts && provisionParts.length > 0) { const provisionStr = provisionParts.join(FIELD_SEPARATOR); hasData = true; return { formattedString: isDebug ? `Provision: ${provisionStr}` : provisionStr, hasData }; } return undefined; }
189-204: Remove or document the commented subProvision codeThis large block of commented code adds noise without explaining why it's preserved but disabled. Either remove it if it's not needed or add a comment explaining why it's being preserved for future implementation.
- // if (provision.provision) { - // const subProvisions = provision.provision - // .map(subProvision => { - // const components = [ - // subProvision.type && `Type: ${subProvision.type}`, - // subProvision.period && formatPeriod(subProvision.period, "Period"), - // subProvision.code && formatCodeableConcepts(subProvision.code, "Code"), - // ].filter(Boolean); - // return components.join(FIELD_SEPARATOR); - // }) - // .filter(Boolean); - - // if (subProvisions.length > 0) { - // provisionParts.push(`Sub-provisions: ${subProvisions.join(FIELD_SEPARATOR)}`); - // } - // } + // TODO: Support nested sub-provisions in a future iteration + // Nested provisions are excluded from search index because [EXPLANATION]
216-216: Consider adding a comment about minimum data checkThe minimum data check is an important control that determines whether the function returns a string or undefined. Consider adding a brief comment to explain the purpose of this check.
- if (!hasMinimumData) return undefined; + // Return undefined if minimum data requirements aren't met to ensure + // meaningful search indexing + if (!hasMinimumData) return undefined;packages/core/src/external/fhir/export/string/resources/medication-request.ts (3)
18-19: Consider using early return for better readabilityThe method accumulates a lot of data and then returns undefined at the end if
hasMinimumDatais false. This pattern makes the code harder to follow.Consider checking for required fields early and returning undefined if they're not present. This would make the code clearer about what fields are required:
toString(request: MedicationRequest, isDebug?: boolean): string | undefined { let hasMinimumData = defaultHasMinimumData; + + // Quick check for required fields + const hasCategory = !!request.category?.length; + const hasMedicationCodeable = !!request.medicationCodeableConcept; + const hasMedicationReference = !!request.medicationReference; + const hasDispenseRequest = !!request.dispenseRequest?.quantity || + request.dispenseRequest?.numberOfRepeatsAllowed !== undefined || + !!request.dispenseRequest?.expectedSupplyDuration; + + // Return early if no minimum data is available + if (!hasCategory && !hasMedicationCodeable && !hasMedicationReference && !hasDispenseRequest) { + return undefined; + } + + // We know we have at least one required field + hasMinimumData = true;
138-146: Consider removing commented codeThe commented-out code for
instantiatesCanonicalandinstantiatesUrioccupies space without adding value. If these fields should be excluded from the string conversion, consider adding a comment explaining why they are excluded.-// if (request.instantiatesCanonical) { -// const instCanonical = request.instantiatesCanonical.join(FIELD_SEPARATOR); -// parts.push(isDebug ? `Instantiates Canonical: ${instCanonical}` : instCanonical); -// } - -// if (request.instantiatesUri) { -// const instUri = request.instantiatesUri.join(FIELD_SEPARATOR); -// parts.push(isDebug ? `Instantiates URI: ${instUri}` : instUri); -// } +// Excluded from search index: instantiatesCanonical, instantiatesUri
76-77: Document reason for excluding subject referenceThe subject reference is commented out, but no explanation is provided for why this field is excluded from the string conversion.
-// const subjectStr = formatReference({ reference: request.subject, label: "Subject", isDebug }); -// if (subjectStr) parts.push(subjectStr); +// Subject reference is excluded from the string representation to avoid duplication in search +// indexes since it's already indexed separately
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (47)
packages/core/src/external/fhir/codeable-concept.ts(1 hunks)packages/core/src/external/fhir/export/string/bundle-to-string.ts(6 hunks)packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/communication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/composition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/condition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/consent.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/coverage.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/document-reference.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/encounter.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/family-member-history.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/immunization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/location.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-administration.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-request.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-statement.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/observation.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/organization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/patient.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/practitioner.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/procedure.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/related-person.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts(6 hunks)packages/core/src/external/fhir/export/string/shared/address.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/age.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/annotation.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/attachment.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/code-display.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts(2 hunks)packages/core/src/external/fhir/export/string/shared/coding.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/debug.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/deny.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/human-name.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/identifier.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/narrative.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/period.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/quantity.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/range.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/telecom.ts(1 hunks)packages/core/src/external/fhir/normalization/resources/encounter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/external/fhir/export/string/shared/attachment.ts
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/core/src/external/fhir/export/string/shared/debug.ts
- packages/core/src/external/fhir/normalization/resources/encounter.ts
- packages/core/src/external/fhir/codeable-concept.ts
- packages/core/src/external/fhir/export/string/shared/tests/codeable-concept.test.ts
- packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts
- packages/core/src/external/fhir/export/string/resources/patient.ts
- packages/core/src/external/fhir/export/string/resources/document-reference.ts
- packages/core/src/external/fhir/export/string/resources/organization.ts
- packages/core/src/external/fhir/export/string/resources/procedure.ts
- packages/core/src/external/fhir/export/string/shared/range.ts
- packages/core/src/external/fhir/export/string/resources/location.ts
- packages/core/src/external/fhir/export/string/resources/medication.ts
- packages/core/src/external/fhir/export/string/shared/age.ts
- packages/core/src/external/fhir/export/string/resources/communication.ts
- packages/core/src/external/fhir/export/string/resources/related-person.ts
- packages/core/src/external/fhir/export/string/resources/composition.ts
- packages/core/src/external/fhir/export/string/resources/medication-statement.ts
- packages/core/src/external/fhir/export/string/shared/deny.ts
- packages/core/src/external/fhir/export/string/shared/code-display.ts
- packages/core/src/external/fhir/export/string/shared/period.ts
- packages/core/src/external/fhir/export/string/resources/coverage.ts
- packages/core/src/external/fhir/export/string/shared/identifier.ts
- packages/core/src/external/fhir/export/string/resources/immunization.ts
- packages/core/src/external/fhir/export/string/resources/practitioner.ts
- packages/core/src/external/fhir/export/string/bundle-to-string.ts
- packages/core/src/external/fhir/export/string/shared/human-name.ts
- packages/core/src/external/fhir/export/string/shared/duration.ts
- packages/core/src/external/fhir/export/string/shared/narrative.ts
- packages/core/src/external/fhir/export/string/resources/medication-administration.ts
- packages/core/src/external/fhir/export/string/shared/annotation.ts
- packages/core/src/external/fhir/export/string/resources/encounter.ts
- packages/core/src/external/fhir/export/string/shared/quantity.ts
- packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts
- packages/core/src/external/fhir/export/string/resources/family-member-history.ts
- packages/core/src/external/fhir/export/string/resources/observation.ts
- packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts
- packages/core/src/external/fhir/export/string/shared/telecom.ts
- packages/core/src/external/fhir/export/string/shared/codeable-concept.ts
- packages/core/src/external/fhir/export/string/shared/reference.ts
- packages/core/src/external/fhir/export/string/shared/dosage.ts
- packages/core/src/external/fhir/export/string/shared/coding.ts
- packages/core/src/external/fhir/export/string/resources/medication-dispense.ts
- packages/core/src/external/fhir/export/string/shared/address.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/fhir/export/string/resources/consent.tspackages/core/src/external/fhir/export/string/resources/condition.tspackages/core/src/external/fhir/export/string/resources/medication-request.ts
🧬 Code Graph Analysis (2)
packages/core/src/external/fhir/export/string/resources/condition.ts (10)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(5-18)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/age.ts (1)
formatAge(8-18)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(10-33)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (11)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (2)
formatIdentifiers(5-18)formatIdentifier(29-43)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
formatDosages(7-21)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/duration.ts (1)
formatDuration(8-25)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
🔇 Additional comments (8)
packages/core/src/external/fhir/export/string/resources/condition.ts (3)
21-73: Good implementation of structured formattingThe implementation of structured formatting for FHIR elements is well done, with consistent use of the new formatting utilities that support the
isDebugflag. The modular approach with individual helper functions for different element types improves maintainability.
150-177: Well-structured handling of complex nested arraysThe handling of complex nested arrays like
stageandevidenceis well implemented. The code correctly maps and filters components, joins them with appropriate separators, and adds debug labels when needed.
179-185: Good use of narrative and annotation formattersThe implementation appropriately uses the new
formatAnnotationsandformatNarrativeutilities to handle note and text content with debug support.packages/core/src/external/fhir/export/string/resources/consent.ts (3)
1-16: Import organization looks goodThe imports are organized clearly by separating external packages from internal modules, with formatting utilities appropriately grouped. This follows the project's organization pattern.
20-22: Class implementation matches interface correctlyThe
ConsentToStringclass properly implements theFHIRResourceToString<Consent>interface with the appropriate generic type parameter. ThetoStringmethod signature correctly includes the optional debug parameter.
17-219: Overall implementation is well structuredThe implementation follows a consistent pattern for building a string representation of a FHIR Consent resource. It:
- Handles all relevant fields in a structured way
- Applies uniform formatting for common FHIR elements
- Properly tracks minimum data requirements
- Supports debug mode for more verbose output
Despite suggestions for minor improvements, the core implementation is solid.
packages/core/src/external/fhir/export/string/resources/medication-request.ts (2)
304-304: Great addition of minimum data checkAdding a check for minimum data before returning the formatted string is an excellent improvement that ensures search indexes only include meaningful content.
2-12: Good refactoring with modular formatting utilitiesThe updated imports bring in modular formatting utilities that provide consistent string representation of FHIR elements. This improves code maintainability and ensures consistent formatting across different FHIR resources.
packages/core/src/external/fhir/export/string/resources/medication-request.ts
Show resolved
Hide resolved
|
@coderabbitai update your comments based on the latest commits, and close the ones not applicable any longer. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1815 🏁 Script executed: Length of output: 11481 @leite08 I attempted to retrieve my previous review comments on this PR but was unable to find any through the GitHub API. Based on the PR history, I can see there have been several meaningful changes:
If you can point me to specific comments that need updating or closing, I'd be happy to address them. Alternatively, would you like me to perform a fresh review of the current state of the PR? (_/) |
9caf6fb to
7f1dc1c
Compare
05e9f3a to
062d9ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (1)
28-31: Status field still doesn't contribute tohasMinimumData.Unlike other fields like
code,conclusion, andpresentedForm, the status field does not update thehasMinimumDataflag. For consistency with the handling of other fields, consider whether status should count toward minimum data presence.if (report.status) { const status = emptyIfDenied(report.status); if (status) parts.push(isDebug ? `Status: ${status}` : status); + hasMinimumData = true; }packages/core/src/external/fhir/export/string/resources/condition.ts (5)
1-11: Missing buildDayjs utility import for date formattingTo properly format date/time fields for improved readability and locale consistency, you should import the
buildDayjsutility function.import { Condition, ConditionEvidence, ConditionStage } from "@medplum/fhirtypes"; import { defaultHasMinimumData, FHIRResourceToString } from "../fhir-resource-to-string"; import { formatAge } from "../shared/age"; import { formatAnnotations } from "../shared/annotation"; import { formatCodeableConcept, formatCodeableConcepts } from "../shared/codeable-concept"; import { formatIdentifiers } from "../shared/identifier"; import { formatNarrative } from "../shared/narrative"; import { formatPeriod } from "../shared/period"; import { formatRange } from "../shared/range"; import { formatReference, formatReferences } from "../shared/reference"; import { FIELD_SEPARATOR } from "../shared/separator"; +import { buildDayjs } from "../../../../../../shared/date-time";
17-19: Re-evaluate the "minimum data" rule criteriaThe
hasMinimumDataflag is only set to true whencondition.codeis present, which may exclude legitimate clinical statements that lack a code but include other significant data like category or severity. Consider broadening these criteria.Consider updating this by either:
- Including other fields in the minimum data check, or
- Delegating the minimum data validation to the caller
For example:
toString(condition: Condition, isDebug?: boolean): string | undefined { let hasMinimumData = defaultHasMinimumData; + // Set to true if ANY of the clinically significant fields are present + if (condition.code || condition.category?.length || condition.severity) { + hasMinimumData = true; + } const parts: string[] = [];
82-84: 🛠️ Refactor suggestionRaw date/time values are output without formatting
The raw ISO date string is displayed without formatting, which reduces readability. Use the
buildDayjs()utility to format dates consistently.if (condition.onsetDateTime) { - parts.push(isDebug ? `Onset Date: ${condition.onsetDateTime}` : condition.onsetDateTime); + const formattedDate = buildDayjs(condition.onsetDateTime).format("YYYY-MM-DD"); + parts.push(isDebug ? `Onset Date: ${formattedDate}` : formattedDate); }
103-107: 🛠️ Refactor suggestionFormat abatement date for readability
Similar to the onset date, the abatement date should be formatted for better readability.
if (condition.abatementDateTime) { - parts.push( - isDebug ? `Abatement Date: ${condition.abatementDateTime}` : condition.abatementDateTime - ); + const formattedDate = buildDayjs(condition.abatementDateTime).format("YYYY-MM-DD"); + parts.push(isDebug ? `Abatement Date: ${formattedDate}` : formattedDate); }
132-134: 🛠️ Refactor suggestionFormat recorded date for readability
Like the other dates, the recorded date should be formatted for better readability.
if (condition.recordedDate) { - parts.push(isDebug ? `Recorded Date: ${condition.recordedDate}` : condition.recordedDate); + const formattedDate = buildDayjs(condition.recordedDate).format("YYYY-MM-DD"); + parts.push(isDebug ? `Recorded Date: ${formattedDate}` : formattedDate); }packages/core/src/external/fhir/export/string/resources/medication-request.ts (4)
69-74:⚠️ Potential issueFix inconsistent setting of
hasMinimumDatafor medication referenceThe code sets
hasMinimumData = truewhenmedicationCodeableConceptis formatted (lines 64-67) but doesn't do the same whenmedicationReferenceis present. This can cause valid records with only a medication reference to be incorrectly excluded from search results.Apply this fix:
const medicationRefStr = formatReference({ reference: request.medicationReference, label: "Medication", isDebug, }); -if (medicationRefStr) parts.push(medicationRefStr); +if (medicationRefStr) { + parts.push(medicationRefStr); + hasMinimumData = true; +}
93-94: 🛠️ Refactor suggestionFormat date string for improved readability
Raw ISO date strings are being output without formatting, which reduces readability for users. The
authoredOndate should be formatted to a more human-readable format.-if (request.authoredOn) - parts.push(isDebug ? `Authored On: ${request.authoredOn}` : request.authoredOn); +if (request.authoredOn) { + const formattedDate = buildDayjs(request.authoredOn).format("YYYY-MM-DD"); + parts.push(isDebug ? `Authored On: ${formattedDate}` : formattedDate); +}Don't forget to add the import at the top of the file:
+import { buildDayjs } from "../../../../shared/date";
240-246: 🛠️ Refactor suggestionPrevent duplicate performer entries in output
The code currently formats both
request.performeranddispense.performerindependently, which will produce two identical lines if they reference the same entity, leading to redundant information in the output.-const performerStr = formatReference({ - reference: dispense.performer, - label: "Dispense Performer", - isDebug, -}); -if (performerStr) dispenseParts.push(performerStr); +// Only add dispense performer if it's different from the main performer +if (dispense.performer && + (!request.performer || + request.performer.reference !== dispense.performer.reference || + request.performer.display !== dispense.performer.display)) { + const performerStr = formatReference({ + reference: dispense.performer, + label: "Dispense Performer", + isDebug, + }); + if (performerStr) dispenseParts.push(performerStr); +}
247-251: 🛠️ Refactor suggestionRefine minimum data criteria for dispense request
Setting
hasMinimumData = trueunconditionally for any non-empty dispenseRequest can incorrectly mark records with minimal dispense information as having sufficient data for search indexing.if (dispenseParts.length > 0) { const dispenseStr = dispenseParts.join(FIELD_SEPARATOR); parts.push(isDebug ? `Dispense Request: ${dispenseStr}` : dispenseStr); - hasMinimumData = true; + // Only set hasMinimumData if we have meaningful dispense data + if (dispense.quantity || + dispense.numberOfRepeatsAllowed !== undefined || + dispense.expectedSupplyDuration || + dispense.initialFill) { + hasMinimumData = true; + } }packages/core/src/external/fhir/export/string/resources/observation.ts (1)
13-14: Add minimum data check for consistency with other formattersThe method lacks a minimum data check to return an empty string when there is insufficient data, unlike other resource formatters that have been updated. Add a condition at the start to check if the observation has the minimum required data.
toString(observation: Observation, isDebug?: boolean): string { + if (!observation.code) return ''; const parts: string[] = [];
🧹 Nitpick comments (4)
packages/core/src/external/fhir/export/string/resources/observation.ts (4)
33-38: Consider differentiating Value labels for clarityBoth
valueQuantityandvalueCodeableConceptuse the same label "Value" when in debug mode. If both fields are present in an Observation, this could lead to confusion in the output string.const valueStr = formatQuantity({ quantity: observation.valueQuantity, - label: "Value", + label: "Value Quantity", isDebug, }); if (valueStr) parts.push(valueStr); // ... (referenceRange code) ... const valueCodeableConceptStr = formatCodeableConcept({ concept: observation.valueCodeableConcept, - label: "Value", + label: "Value Concept", isDebug, });Also applies to: 52-57
1-8: Consider importing types more preciselyWhen importing from @medplum/fhirtypes, it's better to import only the specific types you need rather than importing everything available.
- import { Observation, ObservationReferenceRange } from "@medplum/fhirtypes"; + import type { Observation, ObservationReferenceRange } from "@medplum/fhirtypes";This makes it clear these are only type imports and not runtime values.
40-50: Consider handling empty referenceRange array more elegantlyThe current implementation has multiple chained operations and conditionals. This could be simplified for better readability.
- const referenceRanges = observation.referenceRange - ?.map((rr: ObservationReferenceRange) => { - const lowStr = formatQuantity({ quantity: rr.low, label: "Low", isDebug }); - const highStr = formatQuantity({ quantity: rr.high, label: "High", isDebug }); - return [lowStr, highStr].filter(Boolean).join(FIELD_SEPARATOR); - }) - .filter(Boolean); - if (referenceRanges && referenceRanges.length > 0) { - const referenceRangeStr = referenceRanges.join(FIELD_SEPARATOR); - parts.push(isDebug ? `Reference Range: ${referenceRangeStr}` : referenceRangeStr); - } + if (observation.referenceRange?.length) { + const referenceRanges = observation.referenceRange + .map((rr: ObservationReferenceRange) => { + const lowStr = formatQuantity({ quantity: rr.low, label: "Low", isDebug }); + const highStr = formatQuantity({ quantity: rr.high, label: "High", isDebug }); + return [lowStr, highStr].filter(Boolean).join(FIELD_SEPARATOR); + }) + .filter(Boolean); + + if (referenceRanges.length > 0) { + const referenceRangeStr = referenceRanges.join(FIELD_SEPARATOR); + parts.push(isDebug ? `Reference Range: ${referenceRangeStr}` : referenceRangeStr); + } + }
67-70: Consider using consistent formatting for date fieldsThe date fields (
effectiveDateTimeandissued) are formatted differently from other fields. They're added directly to the parts array rather than using a formatting function.Consider creating a generic
formatDateTimehelper function for consistency:// In a shared date-time.ts file export function formatDateTime({ dateTime, label, isDebug }: { dateTime?: string; label?: string; isDebug?: boolean }): string { if (!dateTime) return ''; return isDebug && label ? `${label}: ${dateTime}` : dateTime; } // Then in this file: import { formatDateTime } from "../shared/date-time"; // ... const effectiveDateTimeStr = formatDateTime({ dateTime: observation.effectiveDateTime, label: "Effective", isDebug, }); if (effectiveDateTimeStr) parts.push(effectiveDateTimeStr); const issuedStr = formatDateTime({ dateTime: observation.issued, label: "Issued", isDebug, }); if (issuedStr) parts.push(issuedStr);Also applies to: 73-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (47)
packages/core/src/external/fhir/codeable-concept.ts(1 hunks)packages/core/src/external/fhir/export/string/bundle-to-string.ts(6 hunks)packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/communication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/composition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/condition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/consent.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/coverage.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/document-reference.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/encounter.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/family-member-history.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/immunization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/location.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-administration.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-request.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-statement.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/observation.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/organization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/patient.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/practitioner.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/procedure.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/related-person.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts(6 hunks)packages/core/src/external/fhir/export/string/shared/address.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/age.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/annotation.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/attachment.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/code-display.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts(2 hunks)packages/core/src/external/fhir/export/string/shared/coding.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/debug.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/deny.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/human-name.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/identifier.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/narrative.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/period.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/quantity.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/range.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/telecom.ts(1 hunks)packages/core/src/external/fhir/normalization/resources/encounter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (42)
- packages/core/src/external/fhir/normalization/resources/encounter.ts
- packages/core/src/external/fhir/export/string/shared/debug.ts
- packages/core/src/external/fhir/export/string/shared/tests/codeable-concept.test.ts
- packages/core/src/external/fhir/codeable-concept.ts
- packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts
- packages/core/src/external/fhir/export/string/shared/deny.ts
- packages/core/src/external/fhir/export/string/resources/location.ts
- packages/core/src/external/fhir/export/string/shared/age.ts
- packages/core/src/external/fhir/export/string/shared/quantity.ts
- packages/core/src/external/fhir/export/string/resources/patient.ts
- packages/core/src/external/fhir/export/string/resources/consent.ts
- packages/core/src/external/fhir/export/string/resources/practitioner.ts
- packages/core/src/external/fhir/export/string/shared/duration.ts
- packages/core/src/external/fhir/export/string/resources/organization.ts
- packages/core/src/external/fhir/export/string/shared/code-display.ts
- packages/core/src/external/fhir/export/string/shared/range.ts
- packages/core/src/external/fhir/export/string/resources/procedure.ts
- packages/core/src/external/fhir/export/string/resources/encounter.ts
- packages/core/src/external/fhir/export/string/resources/medication-statement.ts
- packages/core/src/external/fhir/export/string/resources/medication.ts
- packages/core/src/external/fhir/export/string/resources/immunization.ts
- packages/core/src/external/fhir/export/string/bundle-to-string.ts
- packages/core/src/external/fhir/export/string/resources/coverage.ts
- packages/core/src/external/fhir/export/string/resources/document-reference.ts
- packages/core/src/external/fhir/export/string/resources/medication-administration.ts
- packages/core/src/external/fhir/export/string/shared/address.ts
- packages/core/src/external/fhir/export/string/resources/composition.ts
- packages/core/src/external/fhir/export/string/resources/communication.ts
- packages/core/src/external/fhir/export/string/resources/related-person.ts
- packages/core/src/external/fhir/export/string/shared/narrative.ts
- packages/core/src/external/fhir/export/string/shared/period.ts
- packages/core/src/external/fhir/export/string/shared/annotation.ts
- packages/core/src/external/fhir/export/string/shared/reference.ts
- packages/core/src/external/fhir/export/string/resources/family-member-history.ts
- packages/core/src/external/fhir/export/string/resources/medication-dispense.ts
- packages/core/src/external/fhir/export/string/shared/codeable-concept.ts
- packages/core/src/external/fhir/export/string/shared/attachment.ts
- packages/core/src/external/fhir/export/string/shared/coding.ts
- packages/core/src/external/fhir/export/string/shared/dosage.ts
- packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts
- packages/core/src/external/fhir/export/string/shared/telecom.ts
- packages/core/src/external/fhir/export/string/shared/identifier.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/fhir/export/string/resources/condition.tspackages/core/src/external/fhir/export/string/shared/human-name.tspackages/core/src/external/fhir/export/string/resources/observation.tspackages/core/src/external/fhir/export/string/resources/medication-request.tspackages/core/src/external/fhir/export/string/resources/diagnostic-report.ts
🧬 Code Graph Analysis (3)
packages/core/src/external/fhir/export/string/shared/human-name.ts (2)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (10)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (2)
formatIdentifiers(5-18)formatIdentifier(29-43)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
formatDosages(7-21)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/duration.ts (1)
formatDuration(8-25)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (8)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (1)
formatIdentifiers(5-18)packages/core/src/external/fhir/export/string/shared/deny.ts (1)
emptyIfDenied(9-15)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/reference.ts (1)
formatReferences(8-26)packages/core/src/external/fhir/resources/diagnostic-report.ts (1)
presentedFormsToText(6-14)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
🔇 Additional comments (20)
packages/core/src/external/fhir/export/string/shared/human-name.ts (4)
14-31: Well-structured refactoring with improved parameter handling.The refactoring of
formatHumanNamesto use object parameters instead of positional parameters is a great improvement that follows TypeScript best practices. The consistent return behavior (returningundefinedfor empty or invalid inputs) addresses the previous inconsistency noted in past reviews, and the deduplication of formatted names usinguniqis a good addition.One small suggestion for line 23:
- if (!names || names.length < 1) return undefined; + if (!names || !names.length) return undefined;
33-54: Good modularization with the new helper function.The introduction of
formatHumanNameas a separate helper function is an excellent improvement that follows the single responsibility principle. The function properly handles each component of a human name (prefixes, given names, family name, suffixes) and ensures at least one required field (given or family) exists before returning a result.The consistent parameter pattern and return type (
string | undefined) align well with the broader pattern across FHIR string formatting utilities.
46-47: Good validation for required fields.The check for at least one required field (given names or family name) ensures meaningful output and prevents returning empty strings. This is a good practice for data validation.
25-27: Effective filtering and deduplication of formatted names.The combination of mapping, filtering, and deduplication ensures clean, non-redundant output. The filtering with
Booleanelegantly removes any undefined values returned byformatHumanName.packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts (6)
16-17: LGTM! Good use of default constant.The use of the
defaultHasMinimumDataconstant improves consistency across different resource converters and centralizes the default value.
3-9: Imports are well organized and coherent.The imports have been updated to reflect the new helper functions and the refactored approach. This improves modularity by leveraging shared formatting utilities.
33-38: Excellent refactoring to use consistent parameter objects.The code has been consistently refactored to use object parameters with named properties instead of positional parameters. This improves readability, maintainability, and makes the code more resilient to future parameter changes.
Also applies to: 40-45, 65-72, 76-81, 99-104, 106-111, 113-118, 134-141, 143-144, 146-151
23-26: Good documentation of excluded properties.The commented-out sections clearly indicate which FHIR resource properties are intentionally excluded from the string conversion process. This aligns with the PR objectives and helps clarify what content is excluded from the search index.
Also applies to: 48-59, 83-97
61-64: More informative label for effectiveDateTime.The change from "Effective" to "Effective Date" improves clarity in debug mode by being more specific about what type of effectiveness is being referenced.
153-153: LGTM! Proper enforcement of minimum data requirement.Using
hasMinimumDatato determine whether to return a string ensures that resources without minimum required data don't pollute the search index with low-quality entries.packages/core/src/external/fhir/export/string/resources/condition.ts (3)
69-73: Clarify why subject reference is commented outThe subject reference is a critical part of a FHIR Condition resource, yet it's commented out here. If there's a reason for excluding this field from the string representation, consider adding a comment explaining the rationale.
The subject typically indicates the patient to whom the condition applies, which seems like a fundamental piece of information for search and display purposes.
152-156: Good use of functional array transformsThe implementation of the stage mapping with functional array transforms is well done and follows the coding guidelines for functional programming style.
165-177: Well-structured evidence formattingThe evidence formatting is well-structured, using functional programming principles and proper filtering of empty values. This follows best practices for handling potentially complex nested FHIR structures.
packages/core/src/external/fhir/export/string/resources/medication-request.ts (6)
2-12: LGTM: Imports are well-organized and comprehensiveThe imports are well-organized, covering all the necessary formatting utilities from shared modules to handle different types of FHIR elements.
18-20: LGTM: Method signature follows interface contractThe method signature now correctly follows the
FHIRResourceToStringinterface contract by including the optionalisDebugparameter and returningstring | undefined. The introduction ofhasMinimumDatawith a default value ensures consistent behavior across resource converters.
22-46: LGTM: Basic fields formatting follows standardized patternThe formatting of basic fields like identifiers, status, statusReason, intent, category, and priority follows a standardized pattern with consistent debug labeling and error handling.
172-181: LGTM: Notes and dosage instructions formatting is comprehensiveThe formatting of notes and dosage instructions using the shared formatters is comprehensive and follows the established patterns. The use of object parameters with named fields improves code readability.
186-201: LGTM: Initial fill formatting is thoroughThe code thoroughly handles the formatting of initial fill information with appropriate conditionals and debug labels. The approach of collecting parts and then joining them with appropriate separators is consistent with the rest of the codebase.
301-306: LGTM: Final checks and return logicThe narrative text formatting and final check for minimum data before returning are well-implemented. Returning
undefinedwhen minimum data requirements aren't met ensures that incomplete resources don't pollute search results.packages/core/src/external/fhir/export/string/resources/observation.ts (1)
13-84:❓ Verification inconclusive
Consider adding more Observation fields for completeness
The current implementation omits several potentially important fields that might be useful for searching, such as
component,bodySite, andmethod. Consider adding these fields to provide a more complete string representation of the Observation resource.
🏁 Script executed:
#!/bin/bash # Check if other Observation fields are being used in searches elsewhere in the codebase echo "Searching for references to Observation components in the codebase..." rg -A 2 -B 2 "observation\.component" --type=ts echo "Searching for references to Observation bodySite in the codebase..." rg -A 2 -B 2 "observation\.bodySite" --type=ts echo "Searching for references to Observation method in the codebase..." rg -A 2 -B 2 "observation\.method" --type=ts echo "Checking if these fields are used in other search-related code..." rg -A 5 -B 5 "component|bodySite|method" --type=ts packages/core/src/external/fhir/searchLength of output: 892
🏁 Script executed:
#!/bin/bash # List the external/fhir directory structure to locate any search-related code echo "Listing subdirectories in packages/core/src/external/fhir:" find packages/core/src/external/fhir -maxdepth 2 -type d echo # Highlight directories whose names contain "search" echo "Finding search-related directories under external/fhir:" find packages/core/src/external/fhir -type d | rg -i "search" || echo "No 'search' directories found" echo # Search for any search function invocations in FHIR code echo "Scanning for 'search(' usage in external/fhir code:" rg -n "search(" packages/core/src/external/fhir || echo "No 'search(' calls found"Length of output: 1744
🏁 Script executed:
#!/bin/bash # Locate Observation resource definition files in the FHIR resources directory echo "Searching for Observation definitions under packages/core/src/external/fhir/resources..." files=$(find packages/core/src/external/fhir/resources -type f -iname "*observation*.ts" 2>/dev/null) echo "$files" # Print the first 200 lines of each to inspect available fields for f in $files; do echo echo "----- Definition File: $f -----" sed -n '1,200p' "$f" doneLength of output: 290
🏁 Script executed:
#!/bin/bash # Display imports to locate Observation type origin echo "---- observation.ts imports ----" sed -n '1,50p' packages/core/src/external/fhir/export/string/resources/observation.ts echo # Find where the Observation type/interface is declared echo "---- Searching for Observation definition ----" rg -n "export (interface|type) Observation" -t ts echo # If no direct export found, search more generally for 'Observation =' or 'Observation<' patterns rg -n "Observation[[:space:]]*=" -t ts || echo "No 'Observation =' definitions found"Length of output: 6948
Add missing Observation fields for completeness
The current stringifier omits several FHIR-defined Observation properties—namely
component,bodySite, andmethod—which may be important for searching and display. If your importedObservationtype includes these fields, consider:
- Using
formatCodeableConcept(orformatCodeableConcepts) to renderobservation.bodySiteandobservation.method.- Mapping over
observation.component: ObservationComponent[]to format each component’s code and value (similar to howreferenceRangeis handled).File to update:
- packages/core/src/external/fhir/export/string/resources/observation.ts
Example placement (after
interpretationStr):const bodySiteStr = formatCodeableConcept({ concept: observation.bodySite, label: "Body Site", isDebug, }); if (bodySiteStr) parts.push(bodySiteStr); const methodStr = formatCodeableConcept({ concept: observation.method, label: "Method", isDebug, }); if (methodStr) parts.push(methodStr); const componentStrs = observation.component ?.map((c) => { const code = formatCodeableConcept({ concept: c.code, isDebug }); const val = c.valueQuantity ? formatQuantity({ quantity: c.valueQuantity, isDebug }) : formatCodeableConcept({ concept: c.valueCodeableConcept, isDebug }); return [code, val].filter(Boolean).join(FIELD_SEPARATOR); }) .filter(Boolean); if (componentStrs?.length) { const comps = componentStrs.join(FIELD_SEPARATOR); parts.push(isDebug ? `Component: ${comps}` : comps); }Please verify these fields exist on your
Observationtype and adjust formatting utilities as needed.
There was a problem hiding this comment.
Moved from packages/core/src/external/fhir/export/string/shared/codeable-concept.ts
There was a problem hiding this comment.
where is the reaction attribute? That's the most important field in the AllergyIntolerance resource, as it contains both, the substance to which the patient is allergic to, and the manifestation of that allergy. I think it's crucial that we add it to the processing here
There was a problem hiding this comment.
Good catch!
BTW, I was looking at the extractFromReactions, and there we mix substance and manifestations of diff reactions, this doesn't seem right?
There was a problem hiding this comment.
BTW, I was looking at the extractFromReactions, and there we mix substance and manifestations of diff reactions, this doesn't seem right?
Ya, that func seems kinda fishy..
| // if (communication.subject) { | ||
| // const subjectStr = formatReferences([communication.subject], "Subject"); | ||
| // if (subjectStr) parts.push(subjectStr); | ||
| // } |
There was a problem hiding this comment.
what's the deal with this code?
There was a problem hiding this comment.
Do you mean, why is it commented out? If so, see the PR description:
Note: left a bunch of commented out code to convert properties to be explicit about what we're not adding into the index - i.e., what's not available to search for.
Happy to review how we address that - I can just remove them all in the downstream PR.
If not, what do you mean?
There was a problem hiding this comment.
Ya, it's ok, this was just the first commented out code I came across. That's ok.
Maybe at the top of each resource processing, we can add a list of these unused properties, like so:
// The following properties are not used:
// - subject
// - etc.
There was a problem hiding this comment.
Another practical question - hasMinimumData in this case, mostly depends on having the name of the referenced medicine in the reference.display field, which, imo, is almost never there. This means the resulting file is missing the context on the timing of all medications for a patient - we can only see a list of meds, but we have no idea when they were taken and whether they're still in use, unless this data is presented in doctors notes.
Are we ok with this?
Why don't we do the lookup of the referenced resources? i.e. find the Med that the MedStmnt is pointing to, and take its name?
There was a problem hiding this comment.
Similar questions applies to an array of other resources
There was a problem hiding this comment.
Just a naive implementation to get it out of the door. That's the type of improvements I was hoping to get from you! :)
I can address this on a downstream PR. But just FYI:
- I'm not sure we need this anyways - the goal of these functions is to generate a string representation that can be used to search resources to be part of the search result (and they get hydrated to avoid dangling refs)... so we don't need to have the medication and the date they got administered together - we can just always include the data from the medication statement there, so the dates can be searched for.
- Date search is exact, not range-based at this point.
Thoughts?
There was a problem hiding this comment.
the goal of these functions is to generate a string representation that can be used to search resources to be part of the search result
Ya, I guess what I brought up would be besides the point then
7f1dc1c to
f2bdb87
Compare
2f0722c to
6dbe49b
Compare
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
f2bdb87 to
5078703
Compare
8f23a69 to
51b32da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/core/src/external/fhir/export/string/shared/human-name.ts (1)
33-54: Well-implemented helper function for individual name formatting.The new
formatHumanNamefunction follows the same parameter pattern asformatHumanNamesand properly handles the formatting of individual HumanName objects. The function correctly checks for empty cases at various stages and maintains consistency in the debug labeling pattern.Consider using strict equality comparison for readability:
- if (required.length < 1) return undefined; + if (required.length === 0) return undefined; - if (merged.length < 1) return undefined; + if (merged.length === 0) return undefined;packages/core/src/external/fhir/export/string/resources/coverage.ts (3)
15-16: Consider documenting the default value ofdefaultHasMinimumDataThe variable
defaultHasMinimumDatais initialized from an imported value, but its initial value isn't obvious from this file. Consider adding a comment explaining its default value (likelyfalse) to improve code readability.
97-127: Consider refactoring repetitive array handling patternThis pattern of mapping an array, filtering empty values, joining results, and setting minimum data flag appears in both the classes and costs sections. Consider extracting this to a helper function to reduce duplication.
+ private formatArrayWithMinimumData( + array: any[] | undefined, + mapper: (item: any) => string | undefined, + label?: string, + isDebug?: boolean + ): { formattedString?: string; hasMinimumData: boolean } { + if (!array || array.length === 0) return { hasMinimumData: false }; + + const items = array.map(mapper).filter(Boolean); + if (items.length === 0) return { hasMinimumData: false }; + + const joinedStr = items.join(FIELD_SEPARATOR); + const formattedString = isDebug && label ? `${label}: ${joinedStr}` : joinedStr; + + return { formattedString, hasMinimumData: true }; + }Then use it in both the classes and costs sections:
const { formattedString: classesStr, hasMinimumData: hasClasses } = this.formatArrayWithMinimumData( coverage.class, cls => { /* existing mapping logic */ }, "Classes", isDebug ); if (classesStr) { parts.push(classesStr); hasMinimumData = hasClasses; }🧰 Tools
🪛 Biome (1.9.4)
[error] 107-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
115-116: Simplify the exception string formatting logicThe current implementation checks
exStr.length > 0and returns undefined if it's empty, which is then filtered out. This could be simplified.- const exStr = exComponents.join(FIELD_SEPARATOR); - return exStr.length > 0 ? (isDebug ? `Exception: ${exStr}` : exStr) : undefined; + if (exComponents.length === 0) return undefined; + const exStr = exComponents.join(FIELD_SEPARATOR); + return isDebug ? `Exception: ${exStr}` : exStr;packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
43-43: Use optional chaining for conciseness and safetyThe static analysis tool correctly suggests using optional chaining for better code readability and to prevent potential errors.
- doseAndRateStr && doseAndRateStr.join(FIELD_SEPARATOR), + doseAndRateStr?.join(FIELD_SEPARATOR),🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (3)
76-77: Add comment explaining why subject is excludedThe subject reference formatting is commented out without any explanation. Based on previous review comments, this appears to be intentional and consistent across FHIR exporters.
-// const subjectStr = formatReference({ reference: request.subject, label: "Subject", isDebug }); -// if (subjectStr) parts.push(subjectStr); +// Subject is intentionally excluded from search criteria to avoid redundancy in search index +// const subjectStr = formatReference({ reference: request.subject, label: "Subject", isDebug }); +// if (subjectStr) parts.push(subjectStr);
138-146: Add comment explaining why instantiates fields are excludedThe instantiatesCanonical and instantiatesUri fields are commented out without explanation. Similar to the subject field, a comment would help clarify the intention.
+// instantiatesCanonical and instantiatesUri are excluded from search criteria // if (request.instantiatesCanonical) { // const instCanonical = request.instantiatesCanonical.join(FIELD_SEPARATOR); // parts.push(isDebug ? `Instantiates Canonical: ${instCanonical}` : instCanonical); // } // if (request.instantiatesUri) { // const instUri = request.instantiatesUri.join(FIELD_SEPARATOR); // parts.push(isDebug ? `Instantiates URI: ${instUri}` : instUri); // }
240-246: Consider handling duplicate performer referencesThe code formats both
request.performeranddispense.performerindependently, which could result in duplicate entries if they reference the same entity.This was identified in a previous review and marked as "fine for now." When you're ready to address it, consider implementing a check to avoid duplicating performer information:
+// Check if dispense.performer matches request.performer to avoid duplication +const isDuplicatePerformer = dispense.performer?.reference === request.performer?.reference; +if (!isDuplicatePerformer) { const performerStr = formatReference({ reference: dispense.performer, label: "Dispense Performer", isDebug, }); if (performerStr) dispenseParts.push(performerStr); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
packages/core/src/external/fhir/codeable-concept.ts(1 hunks)packages/core/src/external/fhir/export/string/bundle-to-string.ts(6 hunks)packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/communication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/composition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/condition.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/consent.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/coverage.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/document-reference.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/encounter.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/family-member-history.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/immunization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/location.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-administration.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-dispense.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-request.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication-statement.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/medication.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/observation.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/organization.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/patient.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/practitioner.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/procedure.ts(1 hunks)packages/core/src/external/fhir/export/string/resources/related-person.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/__tests__/codeable-concept.test.ts(6 hunks)packages/core/src/external/fhir/export/string/shared/address.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/age.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/annotation.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/attachment.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/code-display.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts(2 hunks)packages/core/src/external/fhir/export/string/shared/coding.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/debug.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/deny.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/human-name.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/identifier.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/narrative.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/period.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/quantity.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/range.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/reference.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/telecom.ts(1 hunks)packages/core/src/external/fhir/normalization/resources/encounter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/external/fhir/export/string/shared/attachment.ts
- packages/core/src/external/fhir/export/string/shared/annotation.ts
- packages/core/src/external/fhir/normalization/resources/encounter.ts
🚧 Files skipped from review as they are similar to previous changes (40)
- packages/core/src/external/fhir/export/string/shared/debug.ts
- packages/core/src/external/fhir/export/string/shared/age.ts
- packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts
- packages/core/src/external/fhir/export/string/resources/communication.ts
- packages/core/src/external/fhir/export/string/shared/duration.ts
- packages/core/src/external/fhir/export/string/resources/composition.ts
- packages/core/src/external/fhir/export/string/shared/period.ts
- packages/core/src/external/fhir/export/string/resources/medication-administration.ts
- packages/core/src/external/fhir/export/string/resources/location.ts
- packages/core/src/external/fhir/export/string/shared/range.ts
- packages/core/src/external/fhir/export/string/resources/practitioner.ts
- packages/core/src/external/fhir/export/string/resources/organization.ts
- packages/core/src/external/fhir/export/string/resources/document-reference.ts
- packages/core/src/external/fhir/export/string/shared/code-display.ts
- packages/core/src/external/fhir/export/string/resources/medication-statement.ts
- packages/core/src/external/fhir/export/string/resources/related-person.ts
- packages/core/src/external/fhir/export/string/resources/procedure.ts
- packages/core/src/external/fhir/export/string/shared/deny.ts
- packages/core/src/external/fhir/export/string/resources/condition.ts
- packages/core/src/external/fhir/export/string/shared/tests/codeable-concept.test.ts
- packages/core/src/external/fhir/export/string/shared/narrative.ts
- packages/core/src/external/fhir/export/string/resources/consent.ts
- packages/core/src/external/fhir/export/string/shared/identifier.ts
- packages/core/src/external/fhir/export/string/resources/medication.ts
- packages/core/src/external/fhir/export/string/resources/medication-dispense.ts
- packages/core/src/external/fhir/export/string/resources/immunization.ts
- packages/core/src/external/fhir/export/string/shared/reference.ts
- packages/core/src/external/fhir/export/string/resources/patient.ts
- packages/core/src/external/fhir/export/string/shared/address.ts
- packages/core/src/external/fhir/export/string/resources/encounter.ts
- packages/core/src/external/fhir/export/string/resources/family-member-history.ts
- packages/core/src/external/fhir/export/string/resources/diagnostic-report.ts
- packages/core/src/external/fhir/codeable-concept.ts
- packages/core/src/external/fhir/export/string/resources/observation.ts
- packages/core/src/external/fhir/export/string/bundle-to-string.ts
- packages/core/src/external/fhir/export/string/shared/quantity.ts
- packages/core/src/external/fhir/export/string/shared/codeable-concept.ts
- packages/core/src/external/fhir/export/string/resources/allergy-intolerance.ts
- packages/core/src/external/fhir/export/string/shared/coding.ts
- packages/core/src/external/fhir/export/string/shared/telecom.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/fhir/export/string/resources/medication-request.tspackages/core/src/external/fhir/export/string/shared/human-name.tspackages/core/src/external/fhir/export/string/resources/coverage.tspackages/core/src/external/fhir/export/string/shared/dosage.ts
🧠 Learnings (1)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (2)
Learnt from: leite08
PR: metriport/metriport#3869
File: packages/core/src/external/fhir/export/string/resources/medication-request.ts:250-251
Timestamp: 2025-05-21T16:26:58.701Z
Learning: In the FHIR resource to string converters, the hasMinimumData flag is only read at the end of each function to determine whether to return undefined or the formatted string. It's not used to control behavior within the function.
Learnt from: leite08
PR: metriport/metriport#3869
File: packages/core/src/external/fhir/export/string/resources/medication-request.ts:93-94
Timestamp: 2025-05-21T16:49:07.877Z
Learning: Raw ISO date strings in FHIR resources (like authoredOn in MedicationRequest) should be preserved as-is without additional formatting when converting resources to string representations.
🧬 Code Graph Analysis (3)
packages/core/src/external/fhir/export/string/resources/medication-request.ts (11)
packages/core/src/external/fhir/export/string/fhir-resource-to-string.ts (2)
FHIRResourceToString(8-16)defaultHasMinimumData(3-3)packages/core/src/external/fhir/export/string/shared/identifier.ts (2)
formatIdentifiers(5-19)formatIdentifier(30-44)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (2)
formatCodeableConcept(18-68)formatCodeableConcepts(78-96)packages/core/src/external/fhir/export/string/shared/reference.ts (2)
formatReference(28-41)formatReferences(8-26)packages/core/src/external/fhir/export/string/shared/annotation.ts (1)
formatAnnotations(5-21)packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
formatDosages(8-22)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/duration.ts (1)
formatDuration(8-25)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)packages/core/src/external/fhir/export/string/shared/narrative.ts (1)
formatNarrative(10-28)
packages/core/src/external/fhir/export/string/shared/human-name.ts (2)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/dosage.ts (5)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcept(18-68)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(10-33)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)
🪛 Biome (1.9.4)
packages/core/src/external/fhir/export/string/resources/coverage.ts
[error] 107-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/external/fhir/export/string/shared/dosage.ts
[error] 43-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (11)
packages/core/src/external/fhir/export/string/shared/human-name.ts (5)
1-4: Good choice adding lodash's uniq for deduplication.The inclusion of
uniqfrom lodash is appropriate for removing duplicate formatted names, and importingdefaultIsDebugfrom the shared debug file ensures consistency across the formatting utilities.
5-5: Good use of a named constant for separator.Using a named constant
NAME_SEPARATORfor joining name parts improves code readability and maintainability. This makes it easy to change the separator format in one place if needed.
14-31: Well-structured function refactoring with consistent return types.The refactoring of
formatHumanNamesto use object parameters improves readability and maintainability. The function now consistently returnsundefinedfor empty cases, addressing the inconsistency mentioned in the past review comment. The debug labeling implementation is also clean and consistent.
25-27: Good use of functional programming style.The chain of operations (map → filter → uniq) follows functional programming principles and handles empty cases gracefully. Using
filter(Boolean)is a concise way to remove undefined/null values.
44-50: Good approach to name composition.The approach to building the formatted name by combining prefixes, given and family names, and suffixes is clear and maintainable. The filtering of empty parts ensures clean output without unnecessary separators.
packages/core/src/external/fhir/export/string/resources/coverage.ts (5)
56-59: Commented code clarifies excluded propertiesThe commented-out properties (subscriberId, order, network, subrogation) clearly indicate which fields are intentionally excluded from the string representation. This matches the PR objective of explicitly documenting which properties are excluded from search indexing.
80-95: LGTM: Good implementation for classes formattingThis implementation correctly handles the complexity of the
classarray by mapping each class to a formatted string representation and setting the minimum data flag when classes are present. The code follows functional programming principles with immutable transformations.
107-118: Current exception handling is correctThe static analysis tool suggests using optional chaining (
exception?.map), but your current approach with the logical AND operator (exception && exception.map) is equally valid and more explicit. Both approaches prevent null reference errors by ensuring the map operation only executes when exception exists.🧰 Tools
🪛 Biome (1.9.4)
[error] 107-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1-14: Well structured implementation of FHIR resource to string conversionThis implementation follows the defined interface pattern and effectively organizes the conversion of a FHIR Coverage resource to a string representation. The imports are properly organized, and the class is clearly documented with appropriate JSDoc.
139-142: Good use of minimum data validationReturning undefined when minimum data is not present ensures that searches don't match on records with insufficient information. This is a good practice for improving search quality.
packages/core/src/external/fhir/export/string/resources/medication-request.ts (1)
69-74:❓ Verification inconclusive
medicationReference doesn't update hasMinimumData flag
Unlike
medicationCodeableConcept, the code doesn't sethasMinimumDatatotruewhenmedicationReferenceis present. This could result in valid resources with only a medication reference being incorrectly excluded from search results.This issue was identified in a previous review and you mentioned it would be addressed later. If a MedicationRequest resource contains only a reference and none of the other minimum-data fields, the method will incorrectly return
undefined.const medicationRefStr = formatReference({ reference: request.medicationReference, label: "Medication", isDebug, }); -if (medicationRefStr) parts.push(medicationRefStr); +if (medicationRefStr) { + parts.push(medicationRefStr); + hasMinimumData = true; +}Run this script to check if there are MedicationRequest resources that only have medicationReference without medicationCodeableConcept:
🏁 Script executed:
#!/bin/bash # Find MedicationRequest resources with medicationReference but no medicationCodeableConcept ast-grep --pattern $'MedicationRequest { $$$ medicationReference { $$$ } $$$ }' # Check for resources that have medicationReference but not medicationCodeableConcept ast-grep --pattern $'MedicationRequest { $$$ medicationReference { $$$ } $$$ }' | grep -v "medicationCodeableConcept"Length of output: 221
Ensure hasMinimumData is set when medicationReference is present
The formatter currently only pushes
medicationRefStrwithout marking that minimum data requirements have been met. If a MedicationRequest has only a reference (and no codeable concept), it will still be treated as empty. Please update and verify:const medicationRefStr = formatReference({ reference: request.medicationReference, label: "Medication", isDebug, }); -if (medicationRefStr) parts.push(medicationRefStr); +if (medicationRefStr) { + parts.push(medicationRefStr); + hasMinimumData = true; +}• Double-check that no existing MedicationRequest fixtures or live data rely solely on
medicationReferencewithout a codeable concept—if they do, this change is required to avoid dropping valid entries.
• If you have test FHIR resources or sample data, confirm that those with only a reference now pass through as expected.
• Once verified, this diff can be merged to ensure consistent behavior.
…nsolidated-to-text Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
44-44: Use optional chaining for cleaner code.The logical AND check can be simplified using optional chaining as suggested by the static analysis tool.
Apply this diff:
- doseAndRateStr && doseAndRateStr.join(FIELD_SEPARATOR), + doseAndRateStr?.join(FIELD_SEPARATOR),🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/ratio.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/fhir/export/string/shared/ratio.tspackages/core/src/external/fhir/export/string/shared/dosage.ts
🧬 Code Graph Analysis (2)
packages/core/src/external/fhir/export/string/shared/ratio.ts (3)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)
packages/core/src/external/fhir/export/string/shared/dosage.ts (6)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcept(18-68)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(10-33)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/ratio.ts (1)
formatRatio(10-37)
🪛 Biome (1.9.4)
packages/core/src/external/fhir/export/string/shared/dosage.ts
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (2)
packages/core/src/external/fhir/export/string/shared/ratio.ts (1)
10-37: LGTM! Well-implemented formatter following established patterns.The
formatRatiofunction correctly implements the formatting pattern established by other shared formatters likeformatRangeandformatQuantity. It properly handles undefined inputs, formats the numerator and denominator quantities with appropriate labeling, and maintains consistency with the debug mode and field separator usage.packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
9-71: Good implementation following established patterns, but address the timing field issue.The dosage formatting functions follow the established patterns well and correctly use the appropriate formatters for each field type. The past issues with
rateRatiobeing incorrectly passed toformatRangehave been properly resolved by usingformatRatioinstead.However, the critical issue with the
timingfield (line 41) must be addressed before this code can be safely used.🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/external/fhir/export/string/shared/dosage.ts (1)
45-45: Address static analysis suggestion for optional chaining.The static analysis tool correctly identifies that this can be simplified using optional chaining.
Apply this diff to improve readability:
- doseAndRateStr && doseAndRateStr.join(FIELD_SEPARATOR), + doseAndRateStr?.join(FIELD_SEPARATOR),🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/external/fhir/export/string/shared/dosage.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/timing.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/fhir/export/string/shared/timing.tspackages/core/src/external/fhir/export/string/shared/dosage.ts
🧬 Code Graph Analysis (2)
packages/core/src/external/fhir/export/string/shared/timing.ts (6)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcept(18-68)packages/core/src/external/fhir/export/string/shared/duration.ts (1)
formatDuration(8-25)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(10-33)packages/core/src/external/fhir/export/string/shared/period.ts (1)
formatPeriod(11-25)
packages/core/src/external/fhir/export/string/shared/dosage.ts (7)
packages/core/src/external/fhir/export/string/shared/debug.ts (1)
defaultIsDebug(1-1)packages/core/src/external/fhir/export/string/shared/separator.ts (1)
FIELD_SEPARATOR(4-4)packages/core/src/external/fhir/export/string/shared/timing.ts (1)
formatTiming(9-33)packages/core/src/external/fhir/export/string/shared/codeable-concept.ts (1)
formatCodeableConcept(18-68)packages/core/src/external/fhir/export/string/shared/range.ts (1)
formatRange(10-33)packages/core/src/external/fhir/export/string/shared/quantity.ts (1)
formatQuantity(11-24)packages/core/src/external/fhir/export/string/shared/ratio.ts (1)
formatRatio(10-37)
🪛 Biome (1.9.4)
packages/core/src/external/fhir/export/string/shared/dosage.ts
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/core/src/external/fhir/export/string/shared/timing.ts (1)
9-33: LGTM! Clean implementation following established patterns.The
formatTimingfunction correctly handles undefined inputs, uses proper destructuring, and follows the established debug labeling pattern. The implementation leverages existing formatter functions appropriately.packages/core/src/external/fhir/export/string/shared/dosage.ts (2)
42-42: Excellent! formatTiming integration resolved previous issues.The timing field is now correctly formatted using the dedicated
formatTimingfunction, resolving the previous type mismatch and "[object Object]" output issues mentioned in past reviews.
62-67: Verify all formatter functions are correctly implemented.The code correctly uses the appropriate formatter functions for each data type (Range, Quantity, Ratio, CodeableConcept). Based on past review comments, these type mismatches have been resolved.
Ref eng-268 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
Ref eng-268 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 (5)
packages/core/src/external/fhir/export/string/shared/__tests__/timing.test.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/duration.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/numeric.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/string.ts(1 hunks)packages/core/src/external/fhir/export/string/shared/timing.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/external/fhir/export/string/shared/string.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/external/fhir/export/string/shared/duration.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/fhir/export/string/shared/__tests__/timing.test.tspackages/core/src/external/fhir/export/string/shared/timing.tspackages/core/src/external/fhir/export/string/shared/numeric.ts
🧬 Code Graph Analysis (1)
packages/core/src/external/fhir/export/string/shared/__tests__/timing.test.ts (1)
packages/core/src/external/fhir/export/string/shared/timing.ts (1)
formatTiming(11-35)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/core/src/external/fhir/export/string/shared/__tests__/timing.test.ts (1)
1-186: Excellent comprehensive test coverage!This test suite provides thorough coverage of the
formatTimingfunction with well-structured test cases covering:
- Basic functionality (events, repeat rules, codes)
- Debug mode behavior with proper labeling
- Complex repeat rules with multiple parameters
- Edge cases (empty arrays, undefined values)
- Complete timing objects with all components
The test descriptions are clear and the test data is realistic. Great work on ensuring robust testing!
packages/core/src/external/fhir/export/string/shared/timing.ts (1)
1-119: Well-implemented timing formatter with proper error handling!The implementation demonstrates solid architectural design:
- Clean separation of concerns:
formatTiminghandles the main Timing object whileformatTimingRepeatfocuses on the repeat structure- Robust error handling: Returns
undefinedwhen no meaningful content is available- Consistent formatting: Integrates well with other shared formatting utilities
- Type safety: Proper TypeScript usage with destructuring and optional parameters
- Debug mode support: Consistent with the broader FHIR string conversion framework
The code follows functional programming principles and handles all the complex timing scenarios appropriately. Based on the past review comments, previous issues with type consistency and array handling have been properly addressed.
Dependencies
Description
Review code that converts FHIR resources to string:
Closes https://linear.app/metriport/issue/ENG-42/fhir-resources-to-string
Testing
See downstream.
Release Plan
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Tests