Medicine administration UI #10289
Medicine administration UI #10289
Conversation
…medicine-administration-ui
…medicine-administration-ui
…medicine-administration-ui
…medicine-administration-ui
…medicine-administration-ui
…medicine-administration-ui
…medicine-administration-ui
…medicine-administration-ui
…r Questionnaire prefill
…medicine-administration-ui
WalkthroughThis pull request encompasses a broad set of changes across the application. Localization strings in the English JSON file have been updated with key renames and new entries. New API endpoints for medication administration and request upsert operations have been added. Several new React components and forms for managing medication administration have been introduced, along with utility functions to support time slot management. In addition, cosmetic updates to UI elements, modifications to exported functions, and refinements to type definitions and constants further enhance medication management features while removing an unused component. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User Interface
participant AT as AdministrationTab
participant API as Medication API
participant QC as Query Client
UI->>AT: Request medication administrations
AT->>API: GET /api/v1/patient/{id}/medication/administration/
API-->>AT: Return paginated administrations
AT->>UI: Render medication data
sequenceDiagram
participant User
participant Dialog as MedicineAdminDialog
participant API as Medication API
User->>Dialog: Open dialog and fill in form details
Dialog->>API: POST to upsert medication administration
API-->>Dialog: Return success response
Dialog->>User: Display success toast & close dialog
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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: 11
🔭 Outside diff range comments (3)
src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx (2)
Line range hint
291-297: Add encounterId to the query key for proper cache invalidation.The query won't automatically refetch when the encounter changes because
encounterIdis not included in the query key. This could lead to stale data being displayed.Apply this diff to ensure proper cache invalidation:
const { data: patientSymptoms } = useQuery({ - queryKey: ["symptoms", patientId], + queryKey: ["symptoms", patientId, encounterId], queryFn: query(symptomApi.listSymptoms, { pathParams: { patientId }, queryParams: { encounter: encounterId, limit: 100, }, }), });
Line range hint
300-311: Add missing dependencies to useEffect.The effect's dependency array is missing
updateQuestionnaireResponseCBandquestionnaireResponse.question_id. This could lead to stale closures and potentially cause updates to be lost.Apply this diff to fix the dependency array:
useEffect(() => { if (patientSymptoms?.results) { updateQuestionnaireResponseCB( [ { type: "symptom", value: patientSymptoms.results.map(convertToSymptomRequest), }, ], questionnaireResponse.question_id, ); } - }, [patientSymptoms]); + }, [patientSymptoms, updateQuestionnaireResponseCB, questionnaireResponse.question_id]);src/components/Patient/PatientDetailsTab/index.tsx (1)
Remove redundant
patientIdpropThe
patientIdprop is redundant with the existingidprop. The codebase consistently usesidas the primary identifier, and whenpatientIdis needed, it's derived from eitheridorpatientData.id. To maintain consistency and avoid confusion, use only theidprop.🔗 Analysis chain
Line range hint
13-17: Verify patientId prop usage.The
patientIdprop has been added toPatientProps, but it seems redundant with the existingidprop.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if patientId and id are used differently rg -A 5 "patientId.*=.*props" src/ rg -A 5 "id.*=.*props" src/Length of output: 17668
🧹 Nitpick comments (14)
src/components/ui/button.tsx (1)
45-47: Consider documenting the default variant change.Since this is a breaking change that could affect the appearance of buttons across the application, consider:
- Adding a comment explaining why "primary" is now the default variant
- Updating component documentation if it exists
- Communicating this change to other developers
src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx (1)
87-87: Consider adding error handling for invalid encounter IDs.While the implementation is correct, consider adding validation or error handling for cases where the
encounterIdmight be invalid or empty.export function DiagnosisQuestion({ encounterId, patientId, questionnaireResponse, updateQuestionnaireResponseCB, disabled, }: DiagnosisQuestionProps) { + if (!encounterId?.trim()) { + console.warn('Invalid or empty encounter ID provided'); + } const diagnoses = (questionnaireResponse.values?.[0]?.value as DiagnosisRequest[]) || []; const { data: patientDiagnoses } = useQuery({Also applies to: 101-101
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (1)
81-81: LGTM! Consider documenting the encounter-specific behavior.The implementation correctly adds encounter context to medication requests. Consider adding a comment explaining how encounter filtering affects medication requests visibility.
interface MedicationRequestQuestionProps { patientId: string; questionnaireResponse: QuestionnaireResponse; updateQuestionnaireResponseCB: ( values: ResponseValue[], questionId: string, note?: string, ) => void; disabled?: boolean; + /** + * Encounter ID used to filter medication requests. + * When provided, only shows medications associated with this encounter. + */ encounterId: string; }Also applies to: 89-89, 99-99
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx (3)
43-48: Ensure extensibility for alternate time partitions
The hard-coded time slots could be made configurable if the application needs to adapt to different shift schedules or time block conventions in the future. Storing them in a separate configuration (or retrieving from an endpoint) might allow more flexibility.
65-85: Consider extracting visible slot calculation logic
The memoized calculation of visible time slots is rather involved and could be extracted into a custom hook or separate utility. This approach will improve readability and testability.
549-557: Validate safety checks before administering
The “Administer” button is only shown whenmedication.status === "active"in the current time slot. Ensure that all necessary distribution, ingestion, or cross-verification checks are performed prior to pediatric or high-risk medication administration.src/components/Medicine/MedicationAdministration/utils.ts (1)
26-33: Simplify the dose property assignment.The code has repetitive access to the dose quantity object, which can be simplified.
Apply this diff to reduce repetition:
- dose: medication.dosage_instruction[0]?.dose_and_rate?.dose_quantity && { - value: - medication.dosage_instruction[0]?.dose_and_rate?.dose_quantity?.value, - unit: medication.dosage_instruction[0]?.dose_and_rate?.dose_quantity - ?.unit, - code: medication.dosage_instruction[0]?.dose_and_rate?.dose_quantity - ?.unit, + dose: (() => { + const doseQuantity = medication.dosage_instruction?.[0]?.dose_and_rate?.dose_quantity; + return doseQuantity && { + value: doseQuantity.value, + unit: doseQuantity.unit, + code: doseQuantity.unit, + }; + })(),src/types/emr/medicationAdministration/medicationAdministration.ts (1)
56-96: Consider extracting common properties into a shared interface.The
MedicationAdministrationRequestandMedicationAdministrationReadinterfaces share many common properties. Consider creating a base interface to reduce duplication:interface BaseMedicationAdministration { status: MedicationAdministrationStatus; status_reason?: Code; medication: Code; occurrence_period_start: string; occurrence_period_end?: string; recorded?: string; note?: string; dosage?: { text?: string; site?: Code; route?: Code; method?: Code; dose?: DosageQuantity; rate?: Quantity; }; } export interface MedicationAdministrationRequest extends BaseMedicationAdministration { id?: string; encounter: string; request: string; } export interface MedicationAdministrationRead extends BaseMedicationAdministration { id: string; encounter: string; request: string; }src/components/Medicine/MedicationRequestTable/MedicineItem.tsx (1)
23-74: Enhance accessibility with ARIA labels and better contrast.Consider the following improvements:
- Add
aria-labelto the grid container- Use semantic HTML elements for the medication name (
h3is good)- Ensure text contrast meets WCAG guidelines, especially for
text-muted-foreground- <div className="space-y-1"> + <div className="space-y-1" role="article" aria-label={`Medication details for ${medication.medication?.display}`}>src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx (1)
43-46: Prevent potential race conditions in useEffect.The current useEffect implementation might cause race conditions if initialRequest changes rapidly. Consider adding a cleanup function:
React.useEffect(() => { + let isActive = true; - setAdministrationRequest(initialRequest); + if (isActive) { + setAdministrationRequest(initialRequest); + } + return () => { + isActive = false; + }; }, [initialRequest]);src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (1)
181-195: Consider consolidating date/time picker logic.The start and end time picker implementations share similar logic. Consider extracting this into a reusable component to reduce code duplication.
Also applies to: 197-220
src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx (1)
242-248: Consider adding loading state to submit button.The submit button should show a loading state during mutation to prevent double submissions.
<Button type="submit" className="bg-[#006D4C] hover:bg-[#006D4C]/90" - disabled={selectedMedicines.size === 0} + disabled={selectedMedicines.size === 0 || isSubmitting} > - {t("administer_medicines")} ({selectedMedicines.size}) + {isSubmitting ? t("submitting") : `${t("administer_medicines")} (${selectedMedicines.size})`} </Button>src/types/emr/medicationRequest.ts (1)
95-96: Track technical debt: Remove code property after backend update.The TODO comment indicates this is a temporary change. Create a follow-up ticket to remove this property once the backend administration dosage is updated from
QuantitytoDosageQuantity.Would you like me to create a tracking issue for removing this temporary code once the backend is updated?
src/components/Medicine/MedicationRequestTable/index.tsx (1)
115-219: Excellent UI/UX improvements with comprehensive features.The UI implementation includes:
- Clear tab separation between prescriptions and administration
- Responsive search with instant feedback
- Proper handling of loading and empty states
- Horizontal scrolling for wide content
Consider adding keyboard shortcuts for common actions like searching and clearing the search input.
<input type="text" placeholder="Search medications..." value={searchQuery} onChange={(e) => setSearchQuery(e.target.value)} + onKeyDown={(e) => { + if (e.key === 'Escape') { + setSearchQuery(''); + } + }} className="flex-1 bg-transparent text-sm outline-none placeholder:text-muted-foreground" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
public/locale/en.json(11 hunks)src/Utils/request/api.tsx(2 hunks)src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx(2 hunks)src/components/Medicine/MedicationAdministration/AdministrationTab.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx(1 hunks)src/components/Medicine/MedicationAdministration/utils.ts(1 hunks)src/components/Medicine/MedicationRequestTable/MedicineItem.tsx(1 hunks)src/components/Medicine/MedicationRequestTable/MedicineList.tsx(1 hunks)src/components/Medicine/MedicationRequestTable/index.tsx(3 hunks)src/components/Medicine/MedicationsTable.tsx(1 hunks)src/components/Medicine/MultiValueSetSelect.tsx(1 hunks)src/components/Medicine/utils.ts(1 hunks)src/components/Patient/PatientDetailsTab/index.tsx(3 hunks)src/components/Patient/PatientHome.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/AllergyQuestion.tsx(3 hunks)src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx(3 hunks)src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(2 hunks)src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx(3 hunks)src/components/Questionnaire/QuestionTypes/QuestionInput.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx(3 hunks)src/components/ui/button.tsx(1 hunks)src/pages/Encounters/tabs/EncounterUpdatesTab.tsx(1 hunks)src/types/emr/medicationAdministration/medicationAdministration.ts(3 hunks)src/types/emr/medicationAdministration/medicationAdministrationApi.ts(1 hunks)src/types/emr/medicationRequest.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Cypress Tests
src/components/Patient/PatientDetailsTab/index.tsx
[error] 35-35: TypeScript error: Cannot find name 'Updates'
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (30)
src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx (2)
49-49: LGTM! The encounterId prop is correctly typed.The addition of the required
encounterIdproperty aligns with the PR objective to filter symptoms by encounter.
285-285: LGTM! The encounterId prop is correctly destructured.The prop is properly added to the component's parameter list and matches the interface definition.
src/components/ui/button.tsx (1)
45-45: Verify the impact of changing the default variant.Changing the default variant from "default" to "primary" will affect all instances where the Button component is used without an explicit variant prop. This could lead to unexpected visual changes across the application.
Let's verify the Button component usage in the codebase:
✅ Verification successful
Change appears safe due to explicit variant usage pattern
The codebase demonstrates a consistent pattern where button variants are explicitly specified rather than relying on default values. This significantly reduces the risk of unintended visual changes from modifying the default variant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances of Button component usage without explicit variant prop # Search for Button component imports echo "Button component imports:" rg "import.*Button.*from.*components/ui/button" # Search for Button usage without variant prop # Note: This might have false positives but helps identify potential impacts echo -e "\nButton usage without variant prop:" ast-grep --pattern '<Button $$$>' # Search for Button usage with explicit variants echo -e "\nButton usage with explicit variants:" ast-grep --pattern '<Button variant="$_" $$$>'Length of output: 65794
src/components/Questionnaire/QuestionTypes/QuestionInput.tsx (2)
118-136: LGTM! Implementation aligns with PR objectives.The conditional rendering of medication components based on
encounterIdeffectively addresses the bug where structured responses weren't being filtered by Encounter. The implementation is clean and handles the absence ofencounterIdgracefully.
Line range hint
138-174: Verify if appointment type should check for encounterId.While the implementation for allergy, symptom, diagnosis, and encounter types is consistent and well-structured, the appointment type doesn't follow the same pattern of checking for
encounterId. This might be intentional, but it's worth verifying.Let's verify if other appointment-related components in the codebase require
encounterId:src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx (1)
48-48: LGTM! Props interface updated correctly.The
encounterIdproperty is properly typed as string in the interface.src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx (1)
63-63: LGTM! Implementation matches the pattern.The changes correctly integrate the encounter context into the medication statement queries. The implementation is consistent with other components.
Also applies to: 94-94, 114-114
src/components/Questionnaire/QuestionTypes/AllergyQuestion.tsx (1)
63-63: Verify allergy data filtering behavior.The implementation looks correct, but since allergies are critical for patient safety, we should verify that filtering by encounter doesn't hide important allergy information from other encounters.
Also applies to: 120-120, 131-131
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx (2)
152-164: Validate correct handling of older administration records
The logic accumulates the most recent occurrence date per medication. Confirm that older records aren’t mistakenly excluded from other computations if they need to be displayed or tracked.
313-314: Verify status transitions for discontinuation
When discontinuing a medication, it transitions to"ended". Verify that additional business rules (e.g., reason for discontinuation) are captured if required.src/types/emr/medicationAdministration/medicationAdministrationApi.ts (1)
9-20: Confirm return types of upsert endpoint
TheupsertMedicationAdministrationdefinition indicates the response type as an array ofMedicationAdministrationRequestobjects. Verify it matches the backend's actual response (often newly updated or created resources return read objects or references).src/components/Medicine/MedicationRequestTable/MedicineList.tsx (2)
9-14: LGTM! Well-structured props interface.The
MedicineListPropsinterface is well-defined with clear types and proper optional property marking.
16-40: LGTM! Clean and efficient implementation.The component follows React best practices:
- Proper key usage in map function
- Controlled component pattern for checkboxes
- Clear separation of concerns between selection and display
src/components/Medicine/MedicationAdministration/utils.ts (1)
18-19: Consider timezone handling in date formatting.Both start and end times are set to the same moment using
new Date(). This might need to account for timezone differences and duration.Would you like me to provide an implementation that handles timezones properly?
src/components/Patient/PatientDetailsTab/index.tsx (1)
70-70:⚠️ Potential issueFix TypeScript error: Updates component reference.
The pipeline is failing because the
Updatescomponent is still referenced inpatientTabsbut has been removed.Apply this diff to fix the error:
{ route: "updates", - component: Updates, + component: QuestionnaireResponsesList, },Likely invalid or redundant comment.
src/pages/Encounters/tabs/EncounterUpdatesTab.tsx (1)
49-52: LGTM! Proper prop passing to QuestionnaireResponsesList.The addition of the
patientIdprop aligns with the PR objectives and maintains consistency with other components in the file.src/types/emr/medicationAdministration/medicationAdministration.ts (1)
46-46: LGTM! Type consistency improvement.The change from
QuantitytoDosageQuantityaligns the type with medication request definitions, ensuring consistent typing across the codebase.src/components/Medicine/MedicationsTable.tsx (1)
Line range hint
22-31: LGTM! Good function extraction.Making
getFrequencyDisplayexportable improves code reusability while maintaining its functionality.src/components/Medicine/MultiValueSetSelect.tsx (1)
58-58: LGTM! Proper propagation of disabled state.The addition of the
disabledprop to the Button component correctly propagates the disabled state from parent to child, ensuring consistent UI behavior.src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx (2)
17-18: LGTM! Props interface updated correctly.The changes to make
encounteroptional and add requiredpatientIdimprove component reusability while maintaining type safety.
128-141: LGTM! API parameters updated appropriately.The changes to use
patientIddirectly and make encounter conditional align with the interface changes and PR objectives.src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (1)
45-48: Consider adding validation for lastAdministeredDate.The component accepts
lastAdministeredDateas an optional string but doesn't validate its format before using it withnew Date(). This could lead to runtime errors with invalid dates.src/components/Patient/PatientHome.tsx (1)
139-139: LGTM! Consistent prop passing.The addition of the
patientIdprop enables child components to access the patient identifier, which is necessary for the medication administration functionality.src/Utils/request/api.tsx (1)
657-663: LGTM! Well-structured API endpoint.The new medication administration endpoint follows the established API patterns:
- RESTful path structure
- Consistent with other patient-related endpoints
- Proper typing with
PaginatedResponsesrc/components/Medicine/MedicationRequestTable/index.tsx (2)
21-50: Well-structured EmptyState component with good UX.The EmptyState component is well-designed with:
- Clear prop interface
- Conditional messaging based on search state
- Consistent styling with the UI theme
65-101: Clean implementation of medication status management.Good implementation of medication status handling:
- Separate queries for active and stopped medications
- Efficient state management with
showStopped- Clear separation of concerns in data fetching
public/locale/en.json (4)
752-752: LGTM! Comprehensive status keys for medication administration workflow.The added status keys provide clear and concise states for tracking medication administration:
- "draft": For in-progress administration records
- "ended": For completed administration cycles
- "in_progress": For active administration
- "not_done": For skipped or cancelled administration
- "prescribed": For medications that are prescribed but not yet administered
- "taken": For successfully administered medications
Also applies to: 902-902, 1071-1071, 1386-1386, 1585-1585, 1985-1985
759-759: LGTM! Clear action and search labels.The added keys provide clear labels for medication-related actions:
- "edit_administration": For editing medication administration records
- "search_medicine": For medicine search functionality
Also applies to: 1810-1810
1137-1137: LGTM! Appropriate messages for various states.The added keys provide clear messages for:
- Past time administration confirmation
- Empty states for diagnoses and symptoms
Also applies to: 1322-1322, 1373-1373
Line range hint
752-1985: Verify consistency in terminology.Ensure consistent usage of terms across the application:
- "medicine" vs "medication"
- "administration" vs "administered"
✅ Verification successful
Terminology usage is consistent and follows healthcare standards
The codebase demonstrates intentional and consistent terminology usage:
- Backend/API layer uses formal "medication" terminology aligned with healthcare standards (FHIR)
- Frontend/UI layer uses more user-friendly "medicine" terminology for better user experience
- "Administration" related terms are used consistently across all layers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for inconsistent terminology usage echo "Checking for terminology consistency..." rg -i "medic(ine|ation)" rg -i "administ(ration|ered|er)"Length of output: 71229
| if (administrations?.results?.length) { | ||
| const sortedAdmins = [...administrations.results].sort( | ||
| (a, b) => | ||
| new Date(b.occurrence_period_start).getTime() - | ||
| new Date(a.occurrence_period_start).getTime(), | ||
| ); | ||
|
|
||
| if (!lastModifiedDate) { | ||
| setLastModifiedDate(new Date(sortedAdmins[0].occurrence_period_start)); | ||
| } | ||
| } | ||
| }, [administrations]); |
There was a problem hiding this comment.
Potential stale lastModifiedDate issue
Since lastModifiedDate is only set when no previous date exists, any new administration record after the component mounts won’t update this state (line 210). If tracking the latest modification time is important, consider removing the if (!lastModifiedDate) condition.
- if (!lastModifiedDate) {
+ // Always re-check if the new admin is newer
setLastModifiedDate(new Date(sortedAdmins[0].occurrence_period_start));📝 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.
| if (administrations?.results?.length) { | |
| const sortedAdmins = [...administrations.results].sort( | |
| (a, b) => | |
| new Date(b.occurrence_period_start).getTime() - | |
| new Date(a.occurrence_period_start).getTime(), | |
| ); | |
| if (!lastModifiedDate) { | |
| setLastModifiedDate(new Date(sortedAdmins[0].occurrence_period_start)); | |
| } | |
| } | |
| }, [administrations]); | |
| if (administrations?.results?.length) { | |
| const sortedAdmins = [...administrations.results].sort( | |
| (a, b) => | |
| new Date(b.occurrence_period_start).getTime() - | |
| new Date(a.occurrence_period_start).getTime(), | |
| ); | |
| // Always re-check if the new admin is newer | |
| setLastModifiedDate(new Date(sortedAdmins[0].occurrence_period_start)); | |
| } | |
| }, [administrations]); |
| path: "/api/v1/patient/{patientId}/medication/administration/upsert/", | ||
| method: HttpMethod.POST, | ||
| TRes: Type<MedicationAdministrationRequest[]>, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Support partial failures
Consider whether the upsert endpoint should handle partial errors in case some medication administration requests fail, while others succeed. Returning a more detailed response with success/failure statuses may improve transparency.
| if (dose_range) { | ||
| return `${dose_range.low.value}${dose_range.low.unit.display} - ${dose_range.high.value}${dose_range.high.unit.display}`; | ||
| } else if (dose_quantity) { | ||
| return `${dose_quantity.value} ${dose_quantity.unit.display}`; | ||
| } | ||
|
|
||
| const { dose_quantity } = instruction.dose_and_rate; | ||
| if (!dose_quantity?.value || !dose_quantity.unit) return ""; | ||
|
|
||
| return `${dose_quantity.value} ${dose_quantity.unit.display}`; | ||
| return ""; |
There was a problem hiding this comment.
Handle missing or invalid dosage data
The logic gracefully checks for dose_range or dose_quantity. However, if dose_range.low.value or .high.value is absent or invalid, this may cause runtime errors. A more robust guard or fallback could help prevent potential crashes.
if (dose_range) {
+ if (!dose_range.low?.value || !dose_range.low?.unit || !dose_range.high?.value || !dose_range.high?.unit) {
+ return "";
+ }
return `${dose_range.low.value}${dose_range.low.unit.display} - ${dose_range.high.value}${dose_range.high.unit.display}`;
} else if (dose_quantity) {
+ if (!dose_quantity.value || !dose_quantity.unit) {
+ return "";
+ }
return `${dose_quantity.value} ${dose_quantity.unit.display}`;
}📝 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.
| if (dose_range) { | |
| return `${dose_range.low.value}${dose_range.low.unit.display} - ${dose_range.high.value}${dose_range.high.unit.display}`; | |
| } else if (dose_quantity) { | |
| return `${dose_quantity.value} ${dose_quantity.unit.display}`; | |
| } | |
| const { dose_quantity } = instruction.dose_and_rate; | |
| if (!dose_quantity?.value || !dose_quantity.unit) return ""; | |
| return `${dose_quantity.value} ${dose_quantity.unit.display}`; | |
| return ""; | |
| if (dose_range) { | |
| if (!dose_range.low?.value || !dose_range.low?.unit || !dose_range.high?.value || !dose_range.high?.unit) { | |
| return ""; | |
| } | |
| return `${dose_range.low.value}${dose_range.low.unit.display} - ${dose_range.high.value}${dose_range.high.unit.display}`; | |
| } else if (dose_quantity) { | |
| if (!dose_quantity.value || !dose_quantity.unit) { | |
| return ""; | |
| } | |
| return `${dose_quantity.value} ${dose_quantity.unit.display}`; | |
| } | |
| return ""; |
| site: medication.dosage_instruction[0]?.site, | ||
| route: medication.dosage_instruction[0]?.route, | ||
| method: medication.dosage_instruction[0]?.method, |
There was a problem hiding this comment.
Add bounds checking for dosage_instruction array access.
The code directly accesses dosage_instruction[0] without checking if the array is empty, which could lead to runtime errors.
Apply this diff to add safety checks:
dosage: {
- site: medication.dosage_instruction[0]?.site,
- route: medication.dosage_instruction[0]?.route,
- method: medication.dosage_instruction[0]?.method,
+ site: medication.dosage_instruction?.[0]?.site,
+ route: medication.dosage_instruction?.[0]?.route,
+ method: medication.dosage_instruction?.[0]?.method,📝 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.
| site: medication.dosage_instruction[0]?.site, | |
| route: medication.dosage_instruction[0]?.route, | |
| method: medication.dosage_instruction[0]?.method, | |
| site: medication.dosage_instruction?.[0]?.site, | |
| route: medication.dosage_instruction?.[0]?.route, | |
| method: medication.dosage_instruction?.[0]?.method, |
| {formatDosage(medication.dosage_instruction[0])} | ||
| </p> |
There was a problem hiding this comment.
Add null check for dosage_instruction array access.
Direct access to medication.dosage_instruction[0] could throw if the array is empty. Consider using optional chaining:
- {formatDosage(medication.dosage_instruction[0])}
+ {formatDosage(medication.dosage_instruction?.[0])}📝 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.
| {formatDosage(medication.dosage_instruction[0])} | |
| </p> | |
| {formatDosage(medication.dosage_instruction?.[0])} | |
| </p> |
| }); | ||
| }; | ||
|
|
||
| console.log("patientId", patientId); |
There was a problem hiding this comment.
Remove debug console.log statement.
Remove the debug console.log statement before merging to production.
- console.log("patientId", patientId);| <div className="space-y-2"> | ||
| <Label>{t("status")}</Label> | ||
| <Select | ||
| value={administrationRequest.status} | ||
| onValueChange={(value: MedicationAdministrationStatus) => | ||
| onChange({ ...administrationRequest, status: value }) | ||
| } | ||
| disabled={ | ||
| !!administrationRequest.id && | ||
| administrationRequest.status !== "in_progress" | ||
| } | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue placeholder={t("select_status")} /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {MEDICATION_ADMINISTRATION_STATUS.map((status) => ( | ||
| <SelectItem key={status} value={status}> | ||
| {t(status)} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding status transition validation.
The status selection logic should validate whether the transition from one status to another is valid according to the medication administration workflow.
<Select
value={administrationRequest.status}
onValueChange={(value: MedicationAdministrationStatus) =>
- onChange({ ...administrationRequest, status: value })
+ onChange({
+ ...administrationRequest,
+ status: validateStatusTransition(administrationRequest.status, value)
+ ? value
+ : administrationRequest.status,
+ })
}
disabled={
!!administrationRequest.id &&
administrationRequest.status !== "in_progress"
}
>Committable suggestion skipped: line range outside the PR's diff.
| const now = new Date().toISOString().slice(0, 16); | ||
| onChange({ | ||
| ...administrationRequest, | ||
| occurrence_period_start: now, | ||
| occurrence_period_end: now, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential timezone issue with date handling.
Using toISOString().slice(0, 16) might lead to timezone-related issues. Consider using a more robust date formatting approach.
-const now = new Date().toISOString().slice(0, 16);
+const now = new Date().toISOString();Committable suggestion skipped: line range outside the PR's diff.
| const { mutate: upsertAdministrations } = useMutation({ | ||
| mutationFn: mutate( | ||
| medicationAdministrationApi.upsertMedicationAdministration, | ||
| { | ||
| pathParams: { patientId: patientId }, | ||
| }, | ||
| ), | ||
| onSuccess: () => { | ||
| onOpenChange(false); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for mutation failures.
The mutation success case is handled, but there's no error handling. Consider adding error handling and user feedback.
const { mutate: upsertAdministrations } = useMutation({
mutationFn: mutate(
medicationAdministrationApi.upsertMedicationAdministration,
{
pathParams: { patientId: patientId },
},
),
onSuccess: () => {
onOpenChange(false);
},
+ onError: (error) => {
+ // Handle error and show user feedback
+ console.error('Failed to update administrations:', error);
+ // Consider adding a toast notification here
+ },
});📝 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 { mutate: upsertAdministrations } = useMutation({ | |
| mutationFn: mutate( | |
| medicationAdministrationApi.upsertMedicationAdministration, | |
| { | |
| pathParams: { patientId: patientId }, | |
| }, | |
| ), | |
| onSuccess: () => { | |
| onOpenChange(false); | |
| }, | |
| }); | |
| const { mutate: upsertAdministrations } = useMutation({ | |
| mutationFn: mutate( | |
| medicationAdministrationApi.upsertMedicationAdministration, | |
| { | |
| pathParams: { patientId: patientId }, | |
| }, | |
| ), | |
| onSuccess: () => { | |
| onOpenChange(false); | |
| }, | |
| onError: (error) => { | |
| // Handle error and show user feedback | |
| console.error('Failed to update administrations:', error); | |
| // Consider adding a toast notification here | |
| }, | |
| }); |
| const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
|
|
||
| const administrations = Array.from(selectedMedicines).map((id) => { | ||
| return administrationRequests[id]; | ||
| }); | ||
|
|
||
| upsertAdministrations({ | ||
| datapoints: administrations, | ||
| }); | ||
|
|
||
| onOpenChange(false); | ||
| setSelectedMedicines(new Set()); | ||
| setAdministrationRequests({}); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation before submission.
The submit handler should validate all administration requests before submission to ensure data integrity.
const handleSubmit = async (e: React.FormEvent<HTMLFormElement>) => {
e.preventDefault();
const administrations = Array.from(selectedMedicines).map((id) => {
return administrationRequests[id];
});
+ // Validate all administrations before submission
+ const invalidAdministrations = administrations.filter(
+ (admin) => !isValidAdministrationRequest(admin)
+ );
+ if (invalidAdministrations.length > 0) {
+ // Handle invalid administrations
+ return;
+ }
upsertAdministrations({
datapoints: administrations,
});Committable suggestion skipped: line range outside the PR's diff.
Deploying care-fe with
|
| Latest commit: |
aaebcf8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://49e9c5b0.care-fe.pages.dev |
| Branch Preview URL: | https://medicine-administration-ui.care-fe.pages.dev |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
medicine-administration-ui
|
| Run status |
|
| Run duration | 06m 06s |
| Commit |
|
| Committer | Amjith Titus |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
10
|
| View all changes introduced in this branch ↗︎ | |
|
@nihal467 Needs testing |
…medicine-administration-ui
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (1)
51-67:⚠️ Potential issueFix time validation logic.
The current implementation has several issues:
- Uses authored_on instead of encounter start time for validation
- Lacks proper timezone handling
- Doesn't properly validate against future times
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (4)
29-35: Add encounterStartTime to props interface.The props interface is missing the encounterStartTime prop which is required to implement the time validation requirements.
157-174: Add status transition validation.The status selection should validate whether the transition from one status to another is valid according to the medication administration workflow.
219-236: Add constraints to DateTimePicker components.Both DateTimePicker components should have min/max constraints to prevent invalid selections.
Also applies to: 238-259
187-217: Fix timezone handling in radio group time update.When setting current time for non-past administrations, ensure consistent timezone handling.
| const handleStartTimeChange = (newTime: string) => { | ||
| const date = new Date(newTime); | ||
| const error = validateDateTime(date, true); | ||
|
|
||
| if (error) { | ||
| setStartTimeError(error); | ||
| return; | ||
| } | ||
|
|
||
| setStartTimeError(""); | ||
| onChange({ | ||
| ...administrationRequest, | ||
| occurrence_period_start: newTime, | ||
| occurrence_period_end: newTime, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timezone handling to time change handlers.
The time change handlers need to handle timezones consistently to prevent timezone-related bugs.
Apply this diff to fix the handlers:
const handleStartTimeChange = (newTime: string) => {
+ // Convert to user's timezone for validation
+ const userTz = Intl.DateTimeFormat().resolvedOptions().timeZone;
+ const localDate = utcToZonedTime(new Date(newTime), userTz);
- const date = new Date(newTime);
- const error = validateDateTime(date, true);
+ const error = validateDateTime(localDate, true);
if (error) {
setStartTimeError(error);
return;
}
setStartTimeError("");
+ // Convert back to UTC for storage
+ const utcTime = zonedTimeToUtc(localDate, userTz);
onChange({
...administrationRequest,
- occurrence_period_start: newTime,
- occurrence_period_end: newTime,
+ occurrence_period_start: utcTime.toISOString(),
+ occurrence_period_end: utcTime.toISOString(),
});
};Also applies to: 86-100
|
|
…twork/care_fe into medicine-administration-ui
…medicine-administration-ui
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (3)
40-47:⚠️ Potential issueAdd encounter start time to props interface.
Per PR objectives, users should be restricted from selecting administration dates before encounter start time. The interface is missing this crucial prop.
64-80:⚠️ Potential issueUpdate datetime validation to use encounter start time and handle timezones.
The current validation has two issues:
- Uses
authored_oninstead of encounter start time- Lacks timezone handling which could lead to inconsistencies
257-287:⚠️ Potential issueFix timezone handling for current time.
When setting current time for non-past administrations, ensure consistent timezone handling.
🧹 Nitpick comments (1)
src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx (1)
62-75: Add input validation to prevent runtime errors.The
isTimeInSlotfunction should validate its inputs to handle edge cases gracefully.function isTimeInSlot( date: Date, slot: { date: Date; start: string; end: string }, ): boolean { + if (!(date instanceof Date) || !(slot.date instanceof Date)) { + return false; + } + + const timeFormat = /^([0-1]?[0-9]|2[0-3]):([0-5][0-9])$/; + if (!timeFormat.test(slot.start) || !timeFormat.test(slot.end)) { + return false; + } + const slotStartDate = new Date(slot.date); const [startHour] = slot.start.split(":").map(Number); const [endHour] = slot.end.split(":").map(Number);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(17 hunks)src/components/Medicine/MedicationAdministration/AdministrationTab.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx
- public/locale/en.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx
[error] 122-122: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 137-137: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 137-137: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 146-146: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (1)
227-244: Consider adding status transition validation.The status selection logic should validate whether the transition from one status to another is valid according to the medication administration workflow.
src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx (1)
172-180: 🛠️ Refactor suggestionAdd validation before submitting administrations.
The form submission should validate the administration requests before sending them to the server.
const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { e.preventDefault(); const administrations = Array.from(selectedMedicines).map( (id) => administrationRequests[id], ); + + // Validate all administrations + const invalidAdministrations = administrations.filter(admin => { + const { occurrence_period_start, occurrence_period_end } = admin; + return !occurrence_period_start || + !occurrence_period_end || + new Date(occurrence_period_end) <= new Date(occurrence_period_start); + }); + + if (invalidAdministrations.length > 0) { + toast.error(t("invalid_administration_times")); + return; + } + upsertAdministrations({ datapoints: administrations, }); };Likely invalid or redundant comment.
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx (2)
529-554: LGTM! Time slot navigation is well implemented.The implementation correctly handles:
- Prevention of scrolling before prescription date
- Smooth transition between time slots
- Date changes when crossing day boundaries
555-583: LGTM! Administration request handling is comprehensive.The implementation correctly handles both new and existing administrations, including proper preservation of notes and other fields when editing.
| if (!date) return ""; | ||
| try { | ||
| const dateObj = new Date(date); | ||
| if (isNaN(dateObj.getTime())) return ""; |
There was a problem hiding this comment.
Replace unsafe isNaN with Number.isNaN.
Using global isNaN is unsafe as it attempts type coercion. Use Number.isNaN instead for more predictable behavior.
- if (isNaN(dateObj.getTime())) return "";
+ if (Number.isNaN(dateObj.getTime())) return "";
- if (isNaN(hours) || isNaN(minutes)) return;
+ if (Number.isNaN(hours) || Number.isNaN(minutes)) return;
- if (isNaN(currentDate.getTime())) return;
+ if (Number.isNaN(currentDate.getTime())) return;Also applies to: 137-137, 146-146
🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx
Outdated
Show resolved
Hide resolved
| type="time" | ||
| className="w-[150px]" | ||
| value={formatTime(administrationRequest.occurrence_period_start)} | ||
| onChange={(e) => handleTimeChange(e, true)} | ||
| disabled={!isPastTime || !!administrationRequest.id} | ||
| /> |
There was a problem hiding this comment.
Add time input constraints.
The time inputs should have min/max constraints to prevent invalid selections:
- Prevent selection before encounter start time
- Prevent selection of future times
<Input
type="time"
className="w-[150px]"
value={formatTime(administrationRequest.occurrence_period_start)}
onChange={(e) => handleTimeChange(e, true)}
disabled={!isPastTime || !!administrationRequest.id}
+ min={format(new Date(medication.encounter_start_time), 'HH:mm')}
+ max={format(new Date(), 'HH:mm')}
/>Also applies to: 385-394
| <MedicineAdminSheet | ||
| open={isSheetOpen} | ||
| onOpenChange={(open) => { | ||
| setIsSheetOpen(open); | ||
| if (!open) { | ||
| refetchAdministrations(); | ||
| } | ||
| }} | ||
| medications={medications} | ||
| lastAdministeredDates={lastAdministeredDetails?.dates} | ||
| patientId={patientId} | ||
| encounterId={encounterId} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
💡 Codebase verification
Add date and time validation constraints to MedicineAdminSheet.
- The current implementation does not enforce restrictions on date selection—administration dates can be chosen without checking if they are before the encounter start or in the future.
- No validation logic was found in MedicineAdminForm (or related components) to prevent selecting dates prior to the encounter start or dates beyond the current time.
- Update the MedicineAdminSheet component (or its underlying date picker logic) to enforce these constraints as required.
🔗 Analysis chain
Add date and time validation constraints.
As per requirements, add validation to prevent:
- Selecting administration dates before encounter start time
- Selecting future times
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if date/time validation is implemented in MedicineAdminForm
# Search for date/time validation in MedicineAdminForm
ast-grep --pattern 'function MedicineAdminForm($$$) {
$$$
const [date, setDate] = useState($$$);
$$$
}'
# Search for any existing validation logic
rg -A 5 'setDate|onChange.*date'
Length of output: 21892
| {lastModifiedDate && ( | ||
| <div className="text-xs text-[#6b7280]"> | ||
| {t("last_modified")}{" "} | ||
| {formatDistanceToNow(lastModifiedDate)} {t("ago")} | ||
| </div> | ||
| )} | ||
| </div> | ||
| <div className="flex justify-end items-center bg-gray-50 rounded"> | ||
| <Button | ||
| variant="outline" | ||
| size="icon" | ||
| className="h-8 w-8 text-gray-400 mr-2" | ||
| onClick={handlePreviousSlot} | ||
| disabled={!canGoBack} | ||
| title={ | ||
| !canGoBack ? t("cannot_go_before_prescription_date") : "" | ||
| } | ||
| > | ||
| <CareIcon icon="l-angle-left" className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| {visibleSlots.map((slot) => ( | ||
| <TimeSlotHeader | ||
| key={`${format(slot.date, "yyyy-MM-dd")}-${slot.start}`} | ||
| slot={slot} | ||
| isCurrentSlot={isTimeInSlot(currentDate, slot)} | ||
| isEndSlot={slot.date.getTime() === currentDate.getTime()} | ||
| /> | ||
| ))} | ||
| <div className="flex justify-start items-center px-1 bg-gray-50"> | ||
| <Button | ||
| variant="outline" | ||
| size="icon" | ||
| className="h-8 w-8 text-gray-400" | ||
| onClick={handleNextSlot} | ||
| disabled={isTimeInSlot(currentDate, visibleSlots[3])} | ||
| > | ||
| <CareIcon icon="l-angle-right" className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Main content with borders */} | ||
| <div className="col-span-full grid grid-cols-subgrid divide-x divide-[#e5e7eb] border-l border-r"> | ||
| {/* Headers */} | ||
| <div className="p-4 font-medium text-sm border-t bg-[#F3F4F6] text-secondary-700"> | ||
| {t("medicine")}: | ||
| </div> | ||
| {visibleSlots.map((slot, i) => ( | ||
| <div | ||
| key={`${format(slot.date, "yyyy-MM-dd")}-${slot.start}`} | ||
| className="p-4 font-semibold text-xs text-center border-t relative bg-[#F3F4F6] text-secondary-700" | ||
| > | ||
| {i === endSlotIndex && | ||
| slot.date.getTime() === currentDate.getTime() && ( | ||
| <div className="absolute top-0 left-1/2 -translate-y-1/2 -translate-x-1/2"> | ||
| <div className="h-2 w-2 rounded-full bg-blue-500" /> | ||
| </div> | ||
| )} | ||
| {slot.label} | ||
| </div> | ||
| ))} | ||
| <div className="border-t bg-[#F3F4F6]" /> | ||
|
|
||
| {/* Medication rows */} | ||
| {medications?.map((medication) => ( | ||
| <MedicationRow | ||
| key={medication.id} | ||
| medication={medication} | ||
| visibleSlots={visibleSlots} | ||
| currentDate={currentDate} | ||
| administrations={administrations?.results} | ||
| onAdminister={handleAdminister} | ||
| onEditAdministration={handleEditAdministration} | ||
| onDiscontinue={handleDiscontinue} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
|
|
||
| {stoppedMedications?.results?.length > 0 && ( | ||
| <div | ||
| className="p-4 border-t border-[#e5e7eb] flex items-center gap-2 cursor-pointer hover:bg-gray-50" | ||
| onClick={() => setShowStopped(!showStopped)} | ||
| > | ||
| <CareIcon | ||
| icon={showStopped ? "l-eye-slash" : "l-eye"} | ||
| className="h-4 w-4" | ||
| /> | ||
| <span className="text-sm underline"> | ||
| {showStopped ? t("hide") : t("show")}{" "} | ||
| {`${stoppedMedications?.results?.length} ${t("stopped")}`}{" "} | ||
| {t("prescriptions")} | ||
| </span> | ||
| </div> | ||
| )} | ||
| </Card> | ||
| <ScrollBar orientation="horizontal" /> | ||
| </ScrollArea> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement column and row freezing for better usability.
As per requirements, implement:
- Freeze medicine column when scrolling horizontally
- Freeze time frame row when scrolling vertically
- <ScrollArea className="w-full whitespace-nowrap rounded-md">
+ <div className="w-full rounded-md relative">
+ <div className="sticky top-0 z-20 bg-white">
+ {/* Frozen time frame row */}
+ <div className="grid grid-cols-[minmax(200px,2fr),repeat(4,minmax(140px,1fr)),40px]">
+ {/* Time frame header content */}
+ </div>
+ </div>
+ <ScrollArea className="w-full whitespace-nowrap">
<Card className="w-full border-none shadow-none min-w-[640px]">
- <div className="grid grid-cols-[minmax(200px,2fr),repeat(4,minmax(140px,1fr)),40px]">
+ <div className="grid grid-cols-[minmax(200px,2fr),repeat(4,minmax(140px,1fr)),40px] relative">
+ <div className="sticky left-0 z-10 bg-white">
+ {/* Frozen medicine column */}
+ </div>📝 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.
| <ScrollArea className="w-full whitespace-nowrap rounded-md"> | |
| <Card className="w-full border-none shadow-none min-w-[640px]"> | |
| <div className="grid grid-cols-[minmax(200px,2fr),repeat(4,minmax(140px,1fr)),40px]"> | |
| {/* Top row without vertical borders */} | |
| <div className="col-span-full grid grid-cols-subgrid"> | |
| <div className="flex items-center justify-between p-4 bg-gray-50 border-t border-gray-50"> | |
| <div className="flex items-center gap-2 whitespace-break-spaces"> | |
| {lastModifiedDate && ( | |
| <div className="text-xs text-[#6b7280]"> | |
| {t("last_modified")}{" "} | |
| {formatDistanceToNow(lastModifiedDate)} {t("ago")} | |
| </div> | |
| )} | |
| </div> | |
| <div className="flex justify-end items-center bg-gray-50 rounded"> | |
| <Button | |
| variant="outline" | |
| size="icon" | |
| className="h-8 w-8 text-gray-400 mr-2" | |
| onClick={handlePreviousSlot} | |
| disabled={!canGoBack} | |
| title={ | |
| !canGoBack ? t("cannot_go_before_prescription_date") : "" | |
| } | |
| > | |
| <CareIcon icon="l-angle-left" className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| </div> | |
| {visibleSlots.map((slot) => ( | |
| <TimeSlotHeader | |
| key={`${format(slot.date, "yyyy-MM-dd")}-${slot.start}`} | |
| slot={slot} | |
| isCurrentSlot={isTimeInSlot(currentDate, slot)} | |
| isEndSlot={slot.date.getTime() === currentDate.getTime()} | |
| /> | |
| ))} | |
| <div className="flex justify-start items-center px-1 bg-gray-50"> | |
| <Button | |
| variant="outline" | |
| size="icon" | |
| className="h-8 w-8 text-gray-400" | |
| onClick={handleNextSlot} | |
| disabled={isTimeInSlot(currentDate, visibleSlots[3])} | |
| > | |
| <CareIcon icon="l-angle-right" className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| </div> | |
| {/* Main content with borders */} | |
| <div className="col-span-full grid grid-cols-subgrid divide-x divide-[#e5e7eb] border-l border-r"> | |
| {/* Headers */} | |
| <div className="p-4 font-medium text-sm border-t bg-[#F3F4F6] text-secondary-700"> | |
| {t("medicine")}: | |
| </div> | |
| {visibleSlots.map((slot, i) => ( | |
| <div | |
| key={`${format(slot.date, "yyyy-MM-dd")}-${slot.start}`} | |
| className="p-4 font-semibold text-xs text-center border-t relative bg-[#F3F4F6] text-secondary-700" | |
| > | |
| {i === endSlotIndex && | |
| slot.date.getTime() === currentDate.getTime() && ( | |
| <div className="absolute top-0 left-1/2 -translate-y-1/2 -translate-x-1/2"> | |
| <div className="h-2 w-2 rounded-full bg-blue-500" /> | |
| </div> | |
| )} | |
| {slot.label} | |
| </div> | |
| ))} | |
| <div className="border-t bg-[#F3F4F6]" /> | |
| {/* Medication rows */} | |
| {medications?.map((medication) => ( | |
| <MedicationRow | |
| key={medication.id} | |
| medication={medication} | |
| visibleSlots={visibleSlots} | |
| currentDate={currentDate} | |
| administrations={administrations?.results} | |
| onAdminister={handleAdminister} | |
| onEditAdministration={handleEditAdministration} | |
| onDiscontinue={handleDiscontinue} | |
| /> | |
| ))} | |
| </div> | |
| </div> | |
| {stoppedMedications?.results?.length > 0 && ( | |
| <div | |
| className="p-4 border-t border-[#e5e7eb] flex items-center gap-2 cursor-pointer hover:bg-gray-50" | |
| onClick={() => setShowStopped(!showStopped)} | |
| > | |
| <CareIcon | |
| icon={showStopped ? "l-eye-slash" : "l-eye"} | |
| className="h-4 w-4" | |
| /> | |
| <span className="text-sm underline"> | |
| {showStopped ? t("hide") : t("show")}{" "} | |
| {`${stoppedMedications?.results?.length} ${t("stopped")}`}{" "} | |
| {t("prescriptions")} | |
| </span> | |
| </div> | |
| )} | |
| </Card> | |
| <ScrollBar orientation="horizontal" /> | |
| </ScrollArea> | |
| <div className="w-full rounded-md relative"> | |
| <div className="sticky top-0 z-20 bg-white"> | |
| {/* Frozen time frame row */} | |
| <div className="grid grid-cols-[minmax(200px,2fr),repeat(4,minmax(140px,1fr)),40px]"> | |
| {/* Time frame header content */} | |
| </div> | |
| </div> | |
| <ScrollArea className="w-full whitespace-nowrap"> | |
| <Card className="w-full border-none shadow-none min-w-[640px]"> | |
| <div className="grid grid-cols-[minmax(200px,2fr),repeat(4,minmax(140px,1fr)),40px] relative"> | |
| <div className="sticky left-0 z-10 bg-white"> | |
| {/* Frozen medicine column */} | |
| </div> | |
| {/* Top row without vertical borders */} | |
| <div className="col-span-full grid grid-cols-subgrid"> | |
| <div className="flex items-center justify-between p-4 bg-gray-50 border-t border-gray-50"> | |
| <div className="flex items-center gap-2 whitespace-break-spaces"> | |
| {lastModifiedDate && ( | |
| <div className="text-xs text-[#6b7280]"> | |
| {t("last_modified")}{" "} | |
| {formatDistanceToNow(lastModifiedDate)} {t("ago")} | |
| </div> | |
| )} | |
| </div> | |
| <div className="flex justify-end items-center bg-gray-50 rounded"> | |
| <Button | |
| variant="outline" | |
| size="icon" | |
| className="h-8 w-8 text-gray-400 mr-2" | |
| onClick={handlePreviousSlot} | |
| disabled={!canGoBack} | |
| title={ | |
| !canGoBack ? t("cannot_go_before_prescription_date") : "" | |
| } | |
| > | |
| <CareIcon icon="l-angle-left" className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| </div> | |
| {visibleSlots.map((slot) => ( | |
| <TimeSlotHeader | |
| key={`${format(slot.date, "yyyy-MM-dd")}-${slot.start}`} | |
| slot={slot} | |
| isCurrentSlot={isTimeInSlot(currentDate, slot)} | |
| isEndSlot={slot.date.getTime() === currentDate.getTime()} | |
| /> | |
| ))} | |
| <div className="flex justify-start items-center px-1 bg-gray-50"> | |
| <Button | |
| variant="outline" | |
| size="icon" | |
| className="h-8 w-8 text-gray-400" | |
| onClick={handleNextSlot} | |
| disabled={isTimeInSlot(currentDate, visibleSlots[3])} | |
| > | |
| <CareIcon icon="l-angle-right" className="h-4 w-4" /> | |
| </Button> | |
| </div> | |
| </div> | |
| {/* Main content with borders */} | |
| <div className="col-span-full grid grid-cols-subgrid divide-x divide-[#e5e7eb] border-l border-r"> | |
| {/* Headers */} | |
| <div className="p-4 font-medium text-sm border-t bg-[#F3F4F6] text-secondary-700"> | |
| {t("medicine")}: | |
| </div> | |
| {visibleSlots.map((slot, i) => ( | |
| <div | |
| key={`${format(slot.date, "yyyy-MM-dd")}-${slot.start}`} | |
| className="p-4 font-semibold text-xs text-center border-t relative bg-[#F3F4F6] text-secondary-700" | |
| > | |
| {i === endSlotIndex && | |
| slot.date.getTime() === currentDate.getTime() && ( | |
| <div className="absolute top-0 left-1/2 -translate-y-1/2 -translate-x-1/2"> | |
| <div className="h-2 w-2 rounded-full bg-blue-500" /> | |
| </div> | |
| )} | |
| {slot.label} | |
| </div> | |
| ))} | |
| <div className="border-t bg-[#F3F4F6]" /> | |
| {/* Medication rows */} | |
| {medications?.map((medication) => ( | |
| <MedicationRow | |
| key={medication.id} | |
| medication={medication} | |
| visibleSlots={visibleSlots} | |
| currentDate={currentDate} | |
| administrations={administrations?.results} | |
| onAdminister={handleAdminister} | |
| onEditAdministration={handleEditAdministration} | |
| onDiscontinue={handleDiscontinue} | |
| /> | |
| ))} | |
| </div> | |
| </div> | |
| {stoppedMedications?.results?.length > 0 && ( | |
| <div | |
| className="p-4 border-t border-[#e5e7eb] flex items-center gap-2 cursor-pointer hover:bg-gray-50" | |
| onClick={() => setShowStopped(!showStopped)} | |
| > | |
| <CareIcon | |
| icon={showStopped ? "l-eye-slash" : "l-eye"} | |
| className="h-4 w-4" | |
| /> | |
| <span className="text-sm underline"> | |
| {showStopped ? t("hide") : t("show")}{" "} | |
| {`${stoppedMedications?.results?.length} ${t("stopped")}`}{" "} | |
| {t("prescriptions")} | |
| </span> | |
| </div> | |
| )} | |
| </Card> | |
| <ScrollBar orientation="horizontal" /> | |
| </ScrollArea> | |
| </div> |
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (5)
142-142:⚠️ Potential issueReplace unsafe
isNaNwithNumber.isNaN.Using global
isNaNis unsafe as it attempts type coercion. UseNumber.isNaNinstead for more predictable behavior.- if (isNaN(dateObj.getTime())) return ""; + if (Number.isNaN(dateObj.getTime())) return ""; - if (isNaN(hours) || isNaN(minutes)) return; + if (Number.isNaN(hours) || Number.isNaN(minutes)) return; - if (isNaN(currentDate.getTime())) return; + if (Number.isNaN(currentDate.getTime())) return;Also applies to: 157-157, 166-166
🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
269-284:⚠️ Potential issueFix timezone handling for current time.
When setting current time for non-past administrations, ensure consistent timezone handling to prevent timezone-related bugs.
if (newValue === "no") { - const now = new Date().toISOString(); + // Convert local time to UTC for storage + const userTz = Intl.DateTimeFormat().resolvedOptions().timeZone; + const now = zonedTimeToUtc(new Date(), userTz).toISOString(); onChange({ ...administrationRequest, occurrence_period_start: now, occurrence_period_end: now, }); }
325-338:⚠️ Potential issueAdd date constraints to Calendar components.
Both Calendar components should have min/max constraints to prevent invalid selections:
- Prevent selection before encounter start time
- Prevent selection of future dates
<Calendar mode="single" selected={...} onSelect={...} initialFocus + disabled={(date) => { + const now = new Date(); + const encounterStart = new Date(encounterStartTime); + return date < encounterStart || date > now; + }} />Also applies to: 381-394
341-346:⚠️ Potential issueAdd time input constraints.
The time inputs should have min/max constraints to prevent invalid selections:
- Prevent selection before encounter start time
- Prevent selection of future times
<Input type="time" className="w-[150px]" value={formatTime(administrationRequest.occurrence_period_start)} onChange={(e) => handleTimeChange(e, true)} disabled={!isPastTime || !!administrationRequest.id} + min={format(new Date(encounterStartTime), 'HH:mm')} + max={format(new Date(), 'HH:mm')} />Also applies to: 396-406
239-256: 🛠️ Refactor suggestionConsider adding status transition validation.
The status selection logic should validate whether the transition from one status to another is valid according to the medication administration workflow.
<Select value={administrationRequest.status} onValueChange={(value: MedicationAdministrationStatus) => - onChange({ ...administrationRequest, status: value }) + onChange({ + ...administrationRequest, + status: validateStatusTransition(administrationRequest.status, value) + ? value + : administrationRequest.status, + }) } disabled={ !!administrationRequest.id && administrationRequest.status !== "in_progress" } >
🧹 Nitpick comments (3)
src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx (2)
68-70: Ensure color contrast meets WCAG guidelines.The text color
text-rose-500for the "as needed" indicator might not provide sufficient contrast for accessibility. Consider using a darker shade or adding additional visual indicators.- <span className="text-sm text-rose-500"> + <span className="text-sm text-rose-700 font-medium">
105-291: Well-structured component with room for UI improvements.The component is well-implemented with proper error handling, form validation, and state management. Consider implementing the remaining UI improvements for better usability:
- Sticky headers and columns for better scrolling experience
- Consistent row heights
- Collapsed view for medicine administration beyond a certain count
src/components/Medicine/MedicationRequestTable/index.tsx (1)
102-109: Enhance medication search functionality.The current search only matches against medication names. Consider including additional searchable fields for better usability.
const displayedMedications = medications.filter( (med: MedicationRequestRead) => { if (!searchQuery.trim()) return true; const searchTerm = searchQuery.toLowerCase().trim(); const medicationName = med.medication?.display?.toLowerCase() || ""; - return medicationName.includes(searchTerm); + const dosageInstruction = med.dosageInstruction?.[0]?.text?.toLowerCase() || ""; + const route = med.dosageInstruction?.[0]?.route?.display?.toLowerCase() || ""; + return ( + medicationName.includes(searchTerm) || + dosageInstruction.includes(searchTerm) || + route.includes(searchTerm) + ); }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx(1 hunks)src/components/Medicine/MedicationRequestTable/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx
[error] 142-142: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 157-157: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 157-157: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 166-166: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test
- GitHub Check: CodeQL-Build
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: OSSAR-Scan
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (9)
src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx (4)
1-46: LGTM! Well-structured imports and interfaces.The imports are logically organized, and the interfaces are comprehensive and type-safe.
178-186: Add validation before submission.The submit handler should validate all administration requests before submission to ensure data integrity.
246-263: Implement sticky headers and columns for better usability.As per requirements, the medicine column should be frozen when scrolling right, and the time frame row should be frozen when scrolling down.
276-280: 🛠️ Refactor suggestionRemove redundant event handlers in the submit button.
The button already has
type="submit"and the form has anonSubmithandler, making theonClickhandler redundant and potentially unsafe due to type casting.<Button type="submit" className="bg-[#006D4C] hover:bg-[#006D4C]/90" disabled={ selectedMedicines.size === 0 || isPending || !isAllFormsValid } - onClick={(e) => { - e.preventDefault(); - handleSubmit(e as any); - }} >Likely invalid or redundant comment.
src/components/Medicine/MedicationRequestTable/index.tsx (2)
21-50: Well-structured EmptyState component!The component is well-designed with clear props and good defaults, following React best practices and maintaining consistent UI patterns.
67-93: Add error handling for medication queries.Both active and stopped medication queries lack error handling, which could lead to silent failures.
Add error handling to both queries:
const { data: activeMedications, isLoading: loadingActive } = useQuery({ queryKey: ["medication_requests_active", patientId], queryFn: query(medicationRequestApi.list, { pathParams: { patientId: patientId }, queryParams: { encounter: encounterId, limit: 100, status: ["active", "on-hold", "draft", "unknown"].join(","), }, }), enabled: !!patientId, + onError: (error) => { + console.error('Failed to fetch active medications:', error); + }, }); const { data: stoppedMedications, isLoading: loadingStopped } = useQuery({ queryKey: ["medication_requests_stopped", patientId], queryFn: query(medicationRequestApi.list, { pathParams: { patientId: patientId }, queryParams: { encounter: encounterId, limit: 100, status: ["ended", "completed", "cancelled", "entered_in_error"].join(","), }, }), enabled: !!patientId, + onError: (error) => { + console.error('Failed to fetch stopped medications:', error); + }, });src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (3)
258-267: LGTM!The notes input implementation looks good and should fix the notes update issue mentioned in the PR objectives.
40-48:⚠️ Potential issueAdd encounter start time to props interface.
Per PR objectives, users should be restricted from selecting administration dates before encounter start time. The
encounterStartTimeis missing from the props interface.interface MedicineAdminFormProps { medication: MedicationRequestRead; lastAdministeredDate?: string; lastAdministeredBy?: string; administrationRequest: MedicationAdministrationRequest; onChange: (request: MedicationAdministrationRequest) => void; formId: string; isValid?: (valid: boolean) => void; + encounterStartTime: string; }Likely invalid or redundant comment.
66-82:⚠️ Potential issueUpdate datetime validation to use encounter start time.
The validation currently uses
authored_oninstead of encounter start time, which doesn't align with the PR objectives.const validateDateTime = (date: Date, isStartTime: boolean): string => { const now = new Date(); - const authoredOn = new Date(medication.authored_on); + const encounterStart = new Date(encounterStartTime); const startTime = new Date(administrationRequest.occurrence_period_start); if (date > now) { return t( isStartTime ? "start_time_future_error" : "end_time_future_error", ); } if (isStartTime) { - return date < authoredOn ? t("start_time_before_authored_error") : ""; + return date < encounterStart ? t("start_time_before_encounter_error") : ""; } return date < startTime ? t("end_time_before_start_error") : ""; };Likely invalid or redundant comment.
| <ScrollArea className="h-[calc(100vh-16rem)]"> | ||
| <div className="min-w-[800px]"> | ||
| <div className="p-2"> | ||
| <MedicationsTable medications={displayedMedications} /> | ||
| </div> | ||
| {!!stoppedMedications?.results?.length && ( | ||
| <div | ||
| className="p-4 flex items-center gap-2 cursor-pointer hover:bg-gray-50" | ||
| onClick={() => setShowStopped(!showStopped)} | ||
| > | ||
| <CareIcon | ||
| icon={showStopped ? "l-eye-slash" : "l-eye"} | ||
| className="h-4 w-4" | ||
| /> | ||
| <span className="text-sm underline"> | ||
| {showStopped ? t("hide") : t("show")}{" "} | ||
| {`${stoppedMedications?.results?.length} ${t("stopped")}`}{" "} | ||
| {t("prescriptions")} | ||
| </span> | ||
| </div> | ||
| )} | ||
| </div> | ||
| <ScrollBar orientation="horizontal" /> | ||
| </ScrollArea> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing scroll behavior based on reported issues.
Based on the reported issues in the PR, the scroll implementation needs improvements:
- Medicine column should be frozen when scrolling right
- Time frame row should be frozen when scrolling down
Consider using CSS sticky positioning or a specialized table component that supports frozen columns and rows. For example:
-<ScrollArea className="h-[calc(100vh-16rem)]">
+<ScrollArea className="h-[calc(100vh-16rem)] relative">
<div className="min-w-[800px]">
- <div className="p-2">
+ <div className="p-2 relative">
+ <div className="sticky left-0 z-10 bg-white">
+ {/* Medicine column */}
+ </div>
+ <div className="sticky top-0 z-10 bg-white">
+ {/* Time frame row */}
+ </div>
<MedicationsTable medications={displayedMedications} />
</div>📝 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.
| <ScrollArea className="h-[calc(100vh-16rem)]"> | |
| <div className="min-w-[800px]"> | |
| <div className="p-2"> | |
| <MedicationsTable medications={displayedMedications} /> | |
| </div> | |
| {!!stoppedMedications?.results?.length && ( | |
| <div | |
| className="p-4 flex items-center gap-2 cursor-pointer hover:bg-gray-50" | |
| onClick={() => setShowStopped(!showStopped)} | |
| > | |
| <CareIcon | |
| icon={showStopped ? "l-eye-slash" : "l-eye"} | |
| className="h-4 w-4" | |
| /> | |
| <span className="text-sm underline"> | |
| {showStopped ? t("hide") : t("show")}{" "} | |
| {`${stoppedMedications?.results?.length} ${t("stopped")}`}{" "} | |
| {t("prescriptions")} | |
| </span> | |
| </div> | |
| )} | |
| </div> | |
| <ScrollBar orientation="horizontal" /> | |
| </ScrollArea> | |
| <ScrollArea className="h-[calc(100vh-16rem)] relative"> | |
| <div className="min-w-[800px]"> | |
| <div className="p-2 relative"> | |
| <div className="sticky left-0 z-10 bg-white"> | |
| {/* Medicine column */} | |
| </div> | |
| <div className="sticky top-0 z-10 bg-white"> | |
| {/* Time frame row */} | |
| </div> | |
| <MedicationsTable medications={displayedMedications} /> | |
| </div> | |
| {!!stoppedMedications?.results?.length && ( | |
| <div | |
| className="p-4 flex items-center gap-2 cursor-pointer hover:bg-gray-50" | |
| onClick={() => setShowStopped(!showStopped)} | |
| > | |
| <CareIcon | |
| icon={showStopped ? "l-eye-slash" : "l-eye"} | |
| className="h-4 w-4" | |
| /> | |
| <span className="text-sm underline"> | |
| {showStopped ? t("hide") : t("show")}{" "} | |
| {`${stoppedMedications?.results?.length} ${t("stopped")}`}{" "} | |
| {t("prescriptions")} | |
| </span> | |
| </div> | |
| )} | |
| </div> | |
| <ScrollBar orientation="horizontal" /> | |
| </ScrollArea> |
|
LGTM |
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx
Outdated
Show resolved
Hide resolved
…medicine-administration-ui
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx (2)
52-63:⚠️ Potential issueAdd error handling and validation to mutation.
Based on the PR requirements:
- Add error handling to show user-friendly error messages
- Add time validation to prevent future dates
const { mutate: upsertAdministration, isPending } = useMutation({ mutationFn: mutate( medicationAdministrationApi.upsertMedicationAdministration, { pathParams: { patientId: patientId }, }, ), onSuccess: () => { onOpenChange(false); toast.success(t("medication_administration_saved")); }, + onError: (error) => { + toast.error(t("medication_administration_error")); + }, });
65-69:⚠️ Potential issueAdd time validation before submission.
Based on the PR requirements, add validation to:
- Prevent future dates
- Validate against encounter start time
const handleSubmit = () => { + const now = new Date(); + const administrationDate = new Date(administrationRequest.administeredAt); + + if (administrationDate > now) { + toast.error(t("future_administration_not_allowed")); + return; + } + upsertAdministration({ datapoints: [administrationRequest], }); };src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (2)
325-342:⚠️ Potential issueUpdate Calendar components to use encounter start time.
Both Calendar components use authored_on instead of encounter start time for date constraints.
<Calendar mode="single" selected={...} onSelect={...} initialFocus disabled={(date) => { const now = new Date(); - const encounterStart = new Date(medication.authored_on); + const encounterStart = new Date(encounterStartTime); return date < encounterStart || date > now; }} />Also applies to: 386-403
346-351:⚠️ Potential issueAdd time input constraints.
The time inputs should have min/max constraints to prevent invalid selections.
<Input type="time" className="w-[150px]" value={formatTime(administrationRequest.occurrence_period_start)} onChange={(e) => handleTimeChange(e, true)} disabled={!isPastTime || !!administrationRequest.id} + min={format(new Date(encounterStartTime), 'HH:mm')} + max={format(new Date(), 'HH:mm')} />Also applies to: 407-416
🧹 Nitpick comments (2)
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx (1)
51-91: Consider optimizing date calculations.The utility functions are well-structured, but the date calculations could be more efficient by:
- Caching the date objects instead of creating new ones for each comparison.
- Using timestamps for comparisons instead of Date objects.
function getAdministrationsForTimeSlot( administrations: MedicationAdministration[], medicationId: string, slotDate: Date, start: string, end: string, ): MedicationAdministration[] { + const [startHour] = start.split(":").map(Number); + const [endHour] = end.split(":").map(Number); + const slotStartDate = new Date(slotDate); + const slotEndDate = new Date(slotDate); + slotStartDate.setHours(startHour, 0, 0, 0); + slotEndDate.setHours(endHour, 0, 0, 0); + const startTimestamp = slotStartDate.getTime(); + const endTimestamp = slotEndDate.getTime(); return administrations.filter((admin) => { - const adminDate = new Date(admin.occurrence_period_start); - const slotStartDate = new Date(slotDate); - const slotEndDate = new Date(slotDate); - const [startHour] = start.split(":").map(Number); - const [endHour] = end.split(":").map(Number); - slotStartDate.setHours(startHour, 0, 0, 0); - slotEndDate.setHours(endHour, 0, 0, 0); + const adminTimestamp = new Date(admin.occurrence_period_start).getTime(); return ( admin.request === medicationId && - adminDate >= slotStartDate && - adminDate < slotEndDate + adminTimestamp >= startTimestamp && + adminTimestamp < endTimestamp ); }); }src/types/emr/medicationRequest/medicationRequestApi.ts (1)
11-15: Consider adding JSDoc comments for the upsert endpoint.The endpoint looks good but would benefit from documentation explaining:
- When it creates vs updates records
- The expected request payload structure
- Why it returns an array instead of a single record
+/** + * Upserts medication requests. + * Creates a new request if no ID is provided, otherwise updates existing request. + * @returns Array of created/updated medication requests + */ upsert: { path: "/api/v1/patient/{patientId}/medication/request/upsert/", method: HttpMethod.POST, TRes: Type<MedicationRequestRead[]>, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
public/locale/en.json(18 hunks)src/CAREUI/display/SubHeading.tsx(0 hunks)src/components/Medicine/MedicationAdministration/AdministrationTab.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx(1 hunks)src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/DateTimeQuestion.tsx(2 hunks)src/components/ui/date-picker.tsx(2 hunks)src/components/ui/date-range-picker.tsx(2 hunks)src/types/emr/medicationRequest/medicationRequestApi.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/CAREUI/display/SubHeading.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Medicine/MedicationAdministration/MedicineAdminSheet.tsx
- public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (1)
Learnt from: rithviknishad
PR: ohcnetwork/care_fe#10289
File: src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx:325-338
Timestamp: 2025-02-04T07:25:30.683Z
Learning: Calendar components in medication administration forms should include date constraints using the `disabled` prop to prevent selection of dates before encounter start time and future dates.
🪛 Biome (1.9.4)
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx
[error] 142-142: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 157-157: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 157-157: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 166-166: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (18)
src/components/Medicine/MedicationAdministration/AdministrationTab.tsx (5)
93-124: Well-structured interfaces!The component interfaces are well-defined with clear prop types and good separation of concerns.
252-285: Improve accessibility for medication administration records.The clickable div should be a button for better accessibility. Also, consider adding aria-label for the note icon.
745-758: Add date and time validation constraints.As per requirements, add validation to prevent:
- Selecting administration dates before encounter start time
- Selecting future times
614-720: Implement column and row freezing for better usability.As per requirements, implement:
- Freeze medicine column when scrolling horizontally
- Freeze time frame row when scrolling vertically
495-503: Add error handling for discontinue mutation.The discontinue mutation lacks error handling for failed requests.
src/types/emr/medicationRequest/medicationRequestApi.ts (2)
1-1: LGTM! Good use of HttpMethod enum.Using HttpMethod enum instead of string literals provides better type safety.
5-10: LGTM! Consistent use of HttpMethod enum.The endpoint structure follows RESTful conventions and uses the HttpMethod enum consistently.
src/components/Medicine/MedicationAdministration/MedicineAdminDialog.tsx (3)
24-42: LGTM! Well-structured props interface.Props are well-typed and provide all necessary data for medicine administration.
43-50: LGTM! Clean state management.Good use of React hooks for state management and synchronization with initial data.
71-111: LGTM! Clean and accessible dialog implementation.The UI implementation:
- Follows design system conventions
- Handles loading and validation states
- Uses translations consistently
src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx (2)
40-48:⚠️ Potential issueAdd encounterStartTime to props interface.
The component uses medication.authored_on for time validation, but according to PR objectives, users should be restricted from selecting administration dates before encounter start time.
interface MedicineAdminFormProps { medication: MedicationRequestRead; lastAdministeredDate?: string; lastAdministeredBy?: string; administrationRequest: MedicationAdministrationRequest; onChange: (request: MedicationAdministrationRequest) => void; formId: string; isValid?: (valid: boolean) => void; + encounterStartTime: string; }⛔ Skipped due to learnings
Learnt from: rithviknishad PR: ohcnetwork/care_fe#10289 File: src/components/Medicine/MedicationAdministration/MedicineAdminForm.tsx:325-338 Timestamp: 2025-02-04T07:25:30.683Z Learning: Calendar components in medication administration forms should include date constraints using the `disabled` prop to prevent selection of dates before encounter start time and future dates.
66-82:⚠️ Potential issueFix time validation logic.
The validation has two issues:
- Uses authored_on instead of encounter start time
- Lacks timezone handling
const validateDateTime = (date: Date, isStartTime: boolean): string => { + // Convert to user's timezone for comparison + const userTz = Intl.DateTimeFormat().resolvedOptions().timeZone; + const localDate = utcToZonedTime(date, userTz); const now = new Date(); - const authoredOn = new Date(medication.authored_on); + const encounterStart = utcToZonedTime(new Date(encounterStartTime), userTz); const startTime = new Date(administrationRequest.occurrence_period_start); if (date > now) { return t( isStartTime ? "start_time_future_error" : "end_time_future_error", ); } if (isStartTime) { - return date < authoredOn ? t("start_time_before_authored_error") : ""; + return date < encounterStart ? t("start_time_before_encounter_error") : ""; } return date < startTime ? t("end_time_before_start_error") : ""; };Likely invalid or redundant comment.
src/components/ui/date-picker.tsx (1)
2-2: LGTM! Import added for internationalization.The i18next import is correctly added for localization support.
src/components/ui/date-range-picker.tsx (2)
2-2: LGTM! Import added for internationalization.The i18next import is correctly added for localization support.
55-55: Verify date range restrictions for medicine history.While the localization change is good, based on the PR comments about preventing scrolling beyond medicine start date, consider adding validation to the date range selection.
Consider adding a
minDateprop and passing it to the Calendar component:export function DateRangePicker({ date, onChange, className, + minDate?: Date, }: DateRangePickerProps) { return ( // ... <Calendar mode="range" selected={date} onSelect={onChange} + fromDate={minDate} /> // ... ); }src/components/Questionnaire/QuestionTypes/DateTimeQuestion.tsx (3)
2-2: LGTM! Import added for internationalization.The i18next import is correctly added for localization support.
108-108: LGTM! Text localized correctly.The date format and localization are implemented correctly.
66-84: Add time validation for medicine administration.The time change handler should include validation to prevent selecting times before encounter start and future times.
Consider adding time validation:
const handleTimeChange = (event: React.ChangeEvent<HTMLInputElement>) => { const [hours, minutes] = event.target.value.split(":").map(Number); if (isNaN(hours) || isNaN(minutes)) return; const date = currentValue || new Date(); date.setHours(hours); date.setMinutes(minutes); + const now = new Date(); + const encounterStartTime = /* get from context/props */; + if (date < encounterStartTime || date > now) { + // Show error message or prevent invalid selection + return; + } updateQuestionnaireResponseCB( [{ type: "dateTime", value: date.toISOString(), }], questionnaireResponse.question_id, questionnaireResponse.note, ); };🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 68-68: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| const formatTime = (date: string | undefined) => { | ||
| if (!date) return ""; | ||
| try { | ||
| const dateObj = new Date(date); | ||
| if (isNaN(dateObj.getTime())) return ""; | ||
| return `${dateObj.getHours().toString().padStart(2, "0")}:${dateObj | ||
| .getMinutes() | ||
| .toString() | ||
| .padStart(2, "0")}`; | ||
| } catch { | ||
| return ""; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Replace unsafe isNaN with Number.isNaN.
The code uses the global isNaN function which attempts type coercion. Use Number.isNaN instead for more predictable behavior.
const formatTime = (date: string | undefined) => {
if (!date) return "";
try {
const dateObj = new Date(date);
- if (isNaN(dateObj.getTime())) return "";
+ if (Number.isNaN(dateObj.getTime())) return "";
return `${dateObj.getHours().toString().padStart(2, "0")}:${dateObj
.getMinutes()
.toString()
.padStart(2, "0")}`;
} catch {
return "";
}
};
const handleTimeChange = (
event: React.ChangeEvent<HTMLInputElement>,
isStartTime: boolean,
) => {
const [hours, minutes] = event.target.value.split(":").map(Number);
- if (isNaN(hours) || isNaN(minutes)) return;
+ if (Number.isNaN(hours) || Number.isNaN(minutes)) return;
const dateStr = isStartTime
? administrationRequest.occurrence_period_start
: administrationRequest.occurrence_period_end;
if (!dateStr) return;
const currentDate = new Date(dateStr);
- if (isNaN(currentDate.getTime())) return;
+ if (Number.isNaN(currentDate.getTime())) return;Also applies to: 152-182
🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| @@ -46,7 +47,7 @@ export function DatePicker({ date, onChange, disabled }: DatePickerProps) { | |||
| <span className="hidden sm:block">{format(date, "PPP")}</span> | |||
| </> | |||
| ) : ( | |||
| <span>Pick a date</span> | |||
| <span>{t("pick_a_date")}</span> | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing Medicine Administration Date Restrictions in DatePicker
The date picker currently uses the prop as-is (disabled={disabled}) with no extra logic to prevent selecting dates before the encounter start or future dates. There is also no custom function like validateMedicineAdminDate in the repository. These validations are necessary as per the PR and retrieved learnings.
- Location affected:
src/components/ui/date-picker.tsx— thedisabledprop on<Calendar>should be enhanced to include medicine administration date validations. - Action required: Update the
disabledprop with validation logic (e.g., compare against encounter start time and current date).
🔗 Analysis chain
Verify time selection restrictions for medicine administration.
While the localization change is good, based on the PR comments, this component needs additional validation to restrict users from:
- Selecting administration dates before encounter start time
- Selecting future dates
Consider implementing these validations through the disabled prop. Example implementation:
export function DatePicker({ date, onChange, disabled }: DatePickerProps) {
+ const validateMedicineAdminDate = (date: Date) => {
+ const now = new Date();
+ const encounterStartTime = /* get from context/props */;
+ return date < encounterStartTime || date > now;
+ };
return (
// ...
<Calendar
mode="single"
selected={date}
onSelect={onChange}
- disabled={disabled}
+ disabled={(date) => disabled?.(date) || validateMedicineAdminDate(date)}
/>
// ...
);
}I'm going to run a couple of shell scripts to inspect the src/components/ui/date-picker.tsx file and search for any implementation of medicine administration date restrictions (e.g. custom logic added to the disabled prop) to verify whether the recommended changes have been applied.
Below is the first script that extracts the code around the <Calendar component usage (which is where the disabled prop should be augmented):
And here’s another script that searches the repository for any implementation reference (like a function named validateMedicineAdminDate) that might indicate the medicine administration validations have been added:
Once these scripts run, we’ll be able to confirm whether the disabled-date validations for medicine administration—preventing selection of dates before the encounter start time and future dates—are implemented as suggested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
Script:
#!/bin/bash
echo "----- Extracting Calendar component block from src/components/ui/date-picker.tsx -----"
sed -n '/<Calendar/,/\/>/p' src/components/ui/date-picker.tsxLength of output: 481
Script:
#!/bin/bash
echo "----- Searching for 'validateMedicineAdminDate' references in the repository -----"
rg "validateMedicineAdminDate" .Length of output: 213
|
@amjithtitus09 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
This reverts commit 5d9273d.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Style