Fix #8858: Allow decimal places for pCO2, pO2,. Fix #8859: Add additional unit of measure for pCO2 and pO2. #9044
Conversation
…ork#8859: Add additional unit of measure for pCO2 and pO2.
WalkthroughThe changes in this pull request introduce new utility functions for converting pressure measurements between millimeters of mercury (mmHg) and kilopascals (kPa) in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/components/LogUpdate/Sections/ABGAnalysis.tsx (1)
138-140: Consider improving type safety.The current implementation uses a type assertion which could be avoided with a proper type guard.
- {...(field.unit - ? { unit: field.unit } - : ({ units: field.units } as PropsWithUnits))} + {...(field.unit + ? { unit: field.unit } + : { units: field.units! })}src/components/Form/FormFields/RangeFormField.tsx (2)
Line range hint
29-38: LGTM! Consider enhancing type safety for conversion functions.The exported type definition properly supports the unit conversion requirements. The documentation comments clearly explain the purpose of each conversion function.
Consider making the conversion functions required and adding type constraints to ensure values remain within valid ranges:
export type PropsWithUnits = BaseProps & { unit?: undefined; units: { label: string; - // Fn. to convert field's `value` to the this unit. - conversionFn?: (val: number) => number; - // Fn. to convert user input from this unit to field's `value`. - inversionFn?: (val: number) => number; + // Fn. to convert field's `value` to this unit. + conversionFn: (val: number) => number; + // Fn. to convert user input from this unit to field's `value`. + inversionFn: (val: number) => number; }[]; };
Line range hint
42-65: Consider performance optimization for conversion functions.The value conversion logic is executed on every render. Consider memoizing the conversion results to prevent unnecessary recalculations.
+import { useState, useMemo } from "react"; + export default function RangeFormField(props: Props) { const field = useFormFieldPropsResolver(props); const [unit, setUnit] = useState(getInitialUnit(props)); // Value in current unit - const value = (() => { + const value = useMemo(() => { if (props.value == null) { return; } if (typeof props.value === "string") { return unit.conversionFn(parseFloat(props.value)); } return unit.conversionFn(props.value); - })(); + }, [props.value, unit.conversionFn]);src/Utils/utils.ts (2)
540-542: Add documentation and input validation.While the conversion factor precision is sufficient for 2 decimal place requirements, consider these improvements:
- Add JSDoc documentation explaining the purpose and units
- Add input validation for negative values
+/** + * Converts pressure from millimeters of mercury (mmHg) to kilopascals (kPa) + * @param mmHg - Pressure value in millimeters of mercury (must be non-negative) + * @returns Pressure value in kilopascals + */ export const mmHgToKPa = (mmHg: number) => { + if (mmHg < 0) { + throw new Error("Pressure cannot be negative"); + } return mmHg * 0.133322; };
544-546: Add documentation and input validation.Consider these improvements:
- Add JSDoc documentation explaining the purpose and units
- Add input validation for negative values
+/** + * Converts pressure from kilopascals (kPa) to millimeters of mercury (mmHg) + * @param kPa - Pressure value in kilopascals (must be non-negative) + * @returns Pressure value in millimeters of mercury + */ export const kPaToMmHg = (kPa: number) => { + if (kPa < 0) { + throw new Error("Pressure cannot be negative"); + } return kPa / 0.133322; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Utils/utils.ts(1 hunks)src/components/Form/FormFields/RangeFormField.tsx(1 hunks)src/components/LogUpdate/Sections/ABGAnalysis.tsx(5 hunks)
🔇 Additional comments (6)
src/components/LogUpdate/Sections/ABGAnalysis.tsx (4)
4-4: LGTM: Import statements are properly structured.
The new imports support the unit conversion functionality and type safety requirements.
Also applies to: 11-16
26-37: Verify the clinical appropriateness of the PO2 ranges.
While the technical implementation looks correct, please verify:
- The range of 0-20 kPa is clinically appropriate
- The normal range of 10.5-13.5 kPa is accurate for clinical use
46-57: Verify the PCO2 normal range.
The normal range of 5.1-5.6 kPa seems quite narrow. Typical PCO2 ranges are often wider. Please verify this with clinical requirements.
119-119: LGTM: Type definitions are well-structured.
The type definitions properly support:
- Optional single unit with
unit?: string - Multiple units with conversion functions
Also applies to: 124-128
src/components/Form/FormFields/RangeFormField.tsx (1)
Line range hint 146-166: Verify unit conversion integrity.
The component should validate that the conversion functions are properly paired (i.e., they are inverses of each other) to prevent data loss during unit switching.
Add validation in the unit selection handler:
onChange={(e) => {
const resolved = props.units.find(
(o) => o.label === e.value,
);
if (resolved) {
+ // Validate conversion functions are inverses
+ if (resolved.conversionFn && resolved.inversionFn) {
+ const testValue = 100;
+ const roundTrip = resolved.inversionFn(resolved.conversionFn(testValue));
+ if (Math.abs(testValue - roundTrip) > 0.0001) {
+ console.warn('Unit conversion functions are not properly paired');
+ }
+ }
setUnit({
label: resolved.label,
conversionFn: resolved.conversionFn ?? unity,
inversionFn: resolved.inversionFn ?? unity,
});
}
}}src/Utils/utils.ts (1)
540-546: Verify the usage of conversion functions.
The conversion functions look good and provide sufficient precision for the 2 decimal place requirement. Let's verify their integration with the ABG analysis component.
✅ Verification successful
Conversion functions are properly integrated and used
The conversion functions are correctly integrated into the ABG analysis component:
- Used for both pCO2 and pO2 measurements
- Properly set up in the units configuration with bidirectional conversion
- Follows a consistent pattern where kPa is the base unit with mmHg conversions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the conversion functions are used in the ABG analysis component
# and verify if there are any other pressure unit conversions in the codebase.
echo "Checking usage of conversion functions in ABG analysis component..."
rg -l "mmHgToKPa|kPaToMmHg" --type ts
echo "Checking for other pressure unit conversions..."
rg -l "mmHg.*kPa|kPa.*mmHg" --type ts
Length of output: 398
Script:
#!/bin/bash
# Let's check how these conversion functions are used in the ABG analysis component
echo "Checking usage context in ABGAnalysis.tsx..."
rg "mmHgToKPa|kPaToMmHg" -B 2 -A 2 src/components/LogUpdate/Sections/ABGAnalysis.tsx
echo "\nChecking for any unit-related variables or patterns..."
rg "(pO2|pCO2).*(mmHg|kPa)" -B 2 -A 2 src/components/LogUpdate/Sections/ABGAnalysis.tsx
Length of output: 732
Script:
#!/bin/bash
# Let's check the broader context of where these conversions are used in ABGAnalysis
echo "Checking the component structure and unit conversion setup..."
rg "conversionFn|inversionFn" -B 5 -A 5 src/components/LogUpdate/Sections/ABGAnalysis.tsx
echo "\nChecking for pO2 and pCO2 related code..."
ast-grep --pattern 'const $_ = {
$$$
pO2: $_,
$$$
}' src/components/LogUpdate/Sections/ABGAnalysis.tsx
Length of output: 1210
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/LogUpdate/Sections/ABGAnalysis.tsx (2)
119-128: Consider enhancing type safety for unit conversions.While the type structure is good, consider making the conversion functions required when a unit other than the base unit is defined. This would prevent potential runtime errors.
type UnitDefinition = { label: string; conversionFn?: never; inversionFn?: never; } | { label: string; conversionFn: (val: number) => number; inversionFn: (val: number) => number; };
138-140: Improve type safety in prop spreading.The current implementation uses type casting which could be made more type-safe.
- {...(field.unit - ? { unit: field.unit } - : ({ units: field.units } as PropsWithUnits))} + {...(field.unit + ? { unit: field.unit } + : { units: field.units })}This removes the need for type casting while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/LogUpdate/Sections/ABGAnalysis.tsx(5 hunks)
🔇 Additional comments (3)
src/components/LogUpdate/Sections/ABGAnalysis.tsx (3)
4-4: LGTM: Import statements are well-organized.
The new imports support the unit conversion feature and maintain type safety.
Also applies to: 11-16
26-37: Verify the normal ranges for both units.
While the implementation of unit conversion looks correct, please verify that:
- The range of 0-20 kPa is appropriate for both pO2 and pCO2
- The normal ranges in valueDescription (10.5-13.5 for pO2 and 5.1-5.6 for pCO2) are correct for kPa
Consider adding comments to document these medical reference ranges and their source.
Also applies to: 46-57
Line range hint 132-150: Consider adding test cases for unit conversion edge cases.
While the implementation satisfies the requirements, consider adding tests for:
- Edge cases in unit conversion (e.g., handling of zero values, maximum values)
- Precision loss during conversion between units
- Input validation for decimal places in both units
Would you like help creating a test suite for these scenarios?
Proposed Changes
Fix #8858: Allowed decimal places for pCO2, pO2, and pH, enabling more precise input values.
Fix #8859: Added the option to switch units for pCO2 and pO2 between kPa and mmHg on ABG analysis fields.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
ABGAnalysiscomponent to support multiple unit options and conversion functions.Improvements
ABGAnalysiscomponent for better flexibility and usability.PropsWithUnitstype publicly accessible for broader use in other components.Bug Fixes