Dual Valueset for Medication Request Instructions #9996
Conversation
WalkthroughThis pull request introduces comprehensive changes to medication-related components and types across the application. The modifications enhance medication request and statement handling, improve localization for dosage instructions, and introduce new utility functions for formatting medication details. The changes span multiple files, including localization, component implementations, type definitions, and utility functions, focusing on improving the user interface and data management for medication-related interactions. Changes
Sequence DiagramsequenceDiagram
participant User
participant MedicationRequestQuestion
participant MultiValueSetSelect
participant MedicationsTable
User->>MedicationRequestQuestion: Enter medication details
MedicationRequestQuestion->>MultiValueSetSelect: Select instructions/PRN reasons
MultiValueSetSelect-->>MedicationRequestQuestion: Return selected values
MedicationRequestQuestion->>MedicationsTable: Render medication details
MedicationsTable->>User: Display formatted medication information
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Deploying care-fe with
|
| Latest commit: |
c4f7424
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4b10a91a.care-fe.pages.dev |
| Branch Preview URL: | https://dual-valueset-field-medicati.care-fe.pages.dev |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
dual-valueset-field-medication
|
| Run status |
|
| Run duration | 02m 54s |
| Commit |
|
| Committer | Amjith Titus |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
5
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Questionnaire/DualValueSetSelect.tsx (3)
28-39: Consider enhancing type safety and documentation for the interface.The interface is well-structured but could benefit from:
- JSDoc comments explaining each prop's purpose
- Stricter typing for the
systemstuple to ensure exactly 2 elements- Consider using enums or constants for TabIndex instead of magic numbers
+/** Index of the active tab (0 or 1) */ type TabIndex = 0 | 1; +/** Props for the DualValueSetSelect component */ interface DualValueSetSelectProps { + /** Array of exactly two value set systems */ systems: [ValueSetSystem, ValueSetSystem]; + /** Currently selected values for each tab */ values?: [Code | null, Code | null]; + /** Callback when a value is selected in either tab */ onSelect: (index: TabIndex, value: Code | null) => void; placeholders?: [string, string]; disabled?: boolean; count?: number; searchPostFix?: string; labels: [string, string]; }
51-68: Consider optimizing data fetching and state management.The current implementation could benefit from:
- Memoizing the query key array to prevent unnecessary re-renders
- Using a reducer instead of multiple useState hooks
- Adding error handling for the query
+const queryKey = useMemo( + () => ["valueset", systems[activeTab], "expand", count, search], + [systems, activeTab, count, search] +); const searchQuery = useQuery({ - queryKey: ["valueset", systems[activeTab], "expand", count, search], + queryKey, queryFn: query.debounced(routes.valueset.expand, { pathParams: { system: systems[activeTab] }, body: { count, search: search + searchPostFix }, }), + onError: (error) => { + // Handle error appropriately + } });
91-134: Enhance accessibility features in the UI.Consider adding:
- ARIA labels for better screen reader support
- Keyboard navigation between tabs
- Focus management when the popover opens/closes
<Popover open={open} onOpenChange={setOpen}> <PopoverTrigger asChild disabled={disabled}> <Button variant="outline" role="combobox" + aria-label={placeholders[0]} + aria-expanded={open} className={cn( "w-full justify-start h-auto min-h-10 py-2", "text-left", !hasValues && "text-gray-400", )} >src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (1)
652-692: Add error handling for value selection callbacks.The conditional rendering looks good, but consider adding error handling for the onSelect callbacks to handle edge cases gracefully.
onSelect={(index, value) => { + try { if (index === 0) { handleUpdateDosageInstruction({ as_needed_for: value || undefined, }); } else { handleUpdateDosageInstruction({ additional_instruction: value ? [value] : undefined, }); } + } catch (error) { + // Handle error appropriately + console.error('Error updating dosage instruction:', error); + } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Questionnaire/DualValueSetSelect.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (1)
public/locale/en.json (1)
1641-1641: LGTM! Translation key added correctly.The new translation key "prn_reason" is properly added and matches the component's requirements.
…dual-valueset-field-medication
…dual-valueset-field-medication
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (2)
758-790: Consider adding input validation for notes.The notes implementation handles desktop and mobile layouts well, but consider adding:
- Length validation for notes
- Character validation to prevent problematic inputs
<Input value={medication.note || ""} onChange={(e) => onUpdate?.({ note: e.target.value })} placeholder={t("add_notes")} disabled={disabled} className="h-9 text-sm" + maxLength={500} + pattern="[A-Za-z0-9\s.,!?()-]+" />
Line range hint
454-790: Consider extracting the notes input logic into a separate component.The current implementation mixes notes handling with the medication grid row. Consider extracting it into a reusable
MedicationNotescomponent to improve maintainability and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(2 hunks)src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(7 hunks)src/pages/Encounters/PrintPrescription.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/pages/Encounters/PrintPrescription.tsx (1)
208-208: LGTM! Clean variable declaration.The notes variable is appropriately extracted from the medication object.
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (3)
40-41: LGTM! Clean imports.The new components are properly imported.
Line range hint
174-204: LGTM! Grid layout properly updated.The grid layout has been correctly updated to accommodate the new notes column while maintaining proper alignment and responsiveness.
656-696: LGTM! Well-structured dual value set implementation.The conditional rendering between PRN and non-PRN cases is handled cleanly, with proper state management and translations.
✅ Verification successful
Translation keys verified and properly implemented
All translation keys used in the component are present in the locale files with appropriate values:
prn_reasonselect_prn_reasonadditional_instructionsselect_additional_instructions🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify translation keys for PRN reason rg -g '*.json' '"prn_reason":\s*"' public/locale/Length of output: 103
Script:
#!/bin/bash # Description: Verify all related translation keys rg -g '*.json' -e '"prn_reason":\s*"' -e '"select_prn_reason":\s*"' -e '"additional_instructions":\s*"' -e '"select_additional_instructions":\s*"' public/locale/Length of output: 455
| @@ -230,6 +230,7 @@ export const PrintPrescription = (props: { | |||
| </TableCell> | |||
| <TableCell className="py-2 px-3"> | |||
| {remarks || "-"} | |||
| {notes ? ` (${t("note")}: ${notes})` : ""} | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing translations for "note" key in non-English locale files
The "note" translation key exists only in en.json but is missing in:
- public/locale/hi.json
- public/locale/kn.json
- public/locale/ml.json
- public/locale/mr.json
- public/locale/ta.json
🔗 Analysis chain
Verify translation key exists.
The implementation looks good, but ensure the translation key "note" exists in the i18n files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the "note" translation key exists
rg -g '*.json' '"note":\s*"' public/locale/
Length of output: 85
Script:
#!/bin/bash
# List all locale files and check for "note" key in each
fd . 'public/locale' -e json --exec-batch sh -c 'echo "=== Locale files ==="; cat "{}" | jq -r "select(.note != null) | input_filename"'
Length of output: 165
Script:
#!/bin/bash
# List all locale files and show content around "note" key
echo "=== Available locale files ==="
fd . 'public/locale' -e json
echo -e "\n=== Content check for 'note' key ==="
rg -g '*.json' -C 2 '"note":' public/locale/
Length of output: 633
…dual-valueset-field-medication
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/components/Medicine/MedicationsTable.tsx (3)
20-30: Add input validation and error handling to getFrequencyDisplay.The function should validate the timing object structure before accessing properties to prevent potential runtime errors.
function getFrequencyDisplay( timing?: MedicationRequestDosageInstruction["timing"], ) { if (!timing) return undefined; + if (typeof timing !== 'object') return undefined; const code = reverseFrequencyOption(timing); if (!code) return undefined; + if (!(code in MEDICATION_REQUEST_TIMING_OPTIONS)) return undefined; return { code, meaning: MEDICATION_REQUEST_TIMING_OPTIONS[code].display, }; }
33-46: Add type safety to formatDosage function.The function should handle edge cases and provide type safety for the dose_and_rate object.
function formatDosage(instruction: MedicationRequestDosageInstruction) { if (!instruction.dose_and_rate) return ""; + // Ensure dose_and_rate is an object + if (typeof instruction.dose_and_rate !== 'object') return ""; + if (instruction.dose_and_rate.type === "calculated") { const { dose_range } = instruction.dose_and_rate; - if (!dose_range) return ""; + if (!dose_range?.low?.value || !dose_range?.low?.unit?.display || + !dose_range?.high?.value || !dose_range?.high?.unit?.display) return ""; return `${dose_range.low.value}${dose_range.low.unit.display} - ${dose_range.high.value}${dose_range.high.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}`; }
70-72: Add prop validation for medications array.Consider adding runtime validation for the medications prop to ensure data integrity.
+import { z } from "zod"; + +const MedicationSchema = z.object({ + id: z.string(), + medication: z.object({ + display: z.string().optional(), + }).optional(), + dosage_instruction: z.array(z.any()), + status: z.string().optional(), + note: z.string().optional(), +}); + interface MedicationsTableProps { medications: MedicationRequestRead[]; }src/components/Medicine/MedicineAdministrationSheet/index.tsx (2)
Line range hint
44-49: Optimize search filtering performance.Consider memoizing the filtered results to prevent unnecessary re-computations on each render.
+import { useMemo } from "react"; -const filteredMedications = medications?.results?.filter( +const filteredMedications = useMemo(() => medications?.results?.filter( (med: MedicationRequestRead) => { if (!searchQuery.trim()) return true; const searchTerm = searchQuery.toLowerCase().trim(); const medicationName = med.medication?.display?.toLowerCase() || ""; return medicationName.includes(searchTerm); }, - ); + ), [medications?.results, searchQuery]);
147-156: Enhance accessibility for scrollable content.Add ARIA labels to improve screen reader support for the scrollable medication tables.
- <div className="min-w-[800px]"> + <div className="min-w-[800px]" role="region" aria-label="Medications list"> <TabsContent value="active" className="p-2"> <MedicationsTable medications={activeMedications || []} /> </TabsContent> <TabsContent value="discontinued" className="p-2"> <MedicationsTable medications={discontinuedMedications || []} /> </TabsContent> </div>src/pages/Encounters/PrintPrescription.tsx (1)
127-127: Add print-specific styles for MedicationsTable.Consider adding print-specific styles to ensure optimal layout and readability when printed.
- <MedicationsTable medications={medications.results} /> + <div className="print:break-inside-avoid"> + <MedicationsTable + medications={medications.results} + className="print:text-sm print:border-collapse" + /> + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Medicine/MedicationsTable.tsx(1 hunks)src/components/Medicine/MedicineAdministrationSheet/index.tsx(4 hunks)src/pages/Encounters/PrintPrescription.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (1)
src/components/Medicine/MedicationsTable.tsx (1)
124-127: Missing translations for "note" key in non-English locale files.The "note" translation key needs to be added to all locale files.
…dual-valueset-field-medication
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx (1)
471-476: Avoid Duplicate Labels for 'Notes' FieldThe
Labelcomponent for the "notes" field is rendered twice within thedesktopLayoutcondition, causing redundancy.Consider moving the
Labelcomponent outside the conditional rendering to avoid duplication:{/* Notes */} <div className="lg:px-2 lg:py-1 lg:border-r overflow-hidden"> + <Label className="mb-1.5 block text-sm lg:hidden">{t("notes")}</Label> {desktopLayout ? ( <> - <Label className="mb-1.5 block text-sm lg:hidden">{t("notes")}</Label> <Input value={medication.note || ""} onChange={(e) => onUpdate?.({ note: e.target.value })} placeholder={t("add_notes")} disabled={disabled} className="h-9 text-sm" /> </> ) : ( <NotesInput className="mt-2" questionnaireResponse={{ question_id: "", structured_type: "medication_statement", link_id: "", values: [], note: medication.note, }} updateQuestionnaireResponseCB={(response) => { onUpdate?.({ note: response.note }); }} disabled={disabled} /> )} </div>src/types/emr/medicationStatement.ts (1)
43-51: LGTM! Consider adding JSDoc comments.The new
MedicationStatementRequesttype is well-structured and appropriately omits server-side fields (id,patient,encounter). Consider adding JSDoc comments to document the purpose and usage of this type.+/** + * Represents a medication statement request without server-side fields. + * Used for creating new medication statements or updating existing ones. + */ export type MedicationStatementRequest = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/locale/en.json(4 hunks)src/components/Questionnaire/QuestionRenderer.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx(6 hunks)src/components/Questionnaire/structured/types.ts(3 hunks)src/types/emr/medicationStatement.ts(1 hunks)src/types/questionnaire/form.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx (1)
488-499: Verify the Use of Empty Identifiers inNotesInputComponentIn the
NotesInputcomponent, you're passing aquestionnaireResponsewith emptyquestion_idandlink_id. If these identifiers are required by theNotesInputcomponent or its internal logic, this could lead to potential issues.Please confirm that passing empty identifiers does not cause any unintended side effects in the
NotesInputcomponent.src/types/questionnaire/form.ts (1)
Line range hint
5-33: Ensure Consistency After Updating Types fromMedicationStatementtoMedicationStatementRequestThe change from
MedicationStatement[]toMedicationStatementRequest[]in theResponseValuetype is significant. Please verify that all usages ofResponseValuethroughout the codebase are updated accordingly to accommodate this type change.Run the following script to find all usages of
ResponseValueand check for any potential type conflicts:✅ Verification successful
Type Change from
MedicationStatement[]toMedicationStatementRequest[]is Safely IsolatedThe type change is properly contained within the type definition. The codebase handles medication statement responses through dedicated components and type-safe structures, with no direct manipulation of the medication statement types in response processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of ResponseValue and check for MedicationStatement rg 'ResponseValue' -A 5 | rg 'MedicationStatement'Length of output: 48
Script:
#!/bin/bash # Find all ResponseValue usages with context echo "=== ResponseValue usages ===" rg -t typescript "ResponseValue" -B 2 -A 2 # Find type assertions or casts related to ResponseValue echo -e "\n=== Type assertions ===" rg -t typescript "as ResponseValue|<ResponseValue>" -B 2 -A 2 # Check test files specifically echo -e "\n=== Test file usages ===" fd "test|spec" -e ts -e tsx --exec rg -l "ResponseValue" {}Length of output: 433
Script:
#!/bin/bash # Find all TypeScript files containing ResponseValue echo "=== Files using ResponseValue ===" fd -e ts -e tsx --exec rg -l "ResponseValue" {} # Search for both old and new type references echo -e "\n=== MedicationStatement usage ===" fd -e ts -e tsx --exec rg -l "MedicationStatement[^\w]" {} # Look for response handling code with context echo -e "\n=== Response handling context ===" fd -e ts -e tsx --exec rg --context 3 "ResponseValue.*MedicationStatement" {}Length of output: 865
Script:
#!/bin/bash # Check content of key files echo "=== QuestionnaireResponse type content ===" cat src/types/questionnaire/questionnaireResponse.ts echo -e "\n=== QuestionInput component handling ===" cat src/components/Questionnaire/QuestionTypes/QuestionInput.tsxLength of output: 8353
src/components/Questionnaire/structured/types.ts (1)
Line range hint
5-28: Update Usages After Changing Types toMedicationStatementRequestThe types in
StructuredDataMapandStructuredRequestMaphave been updated to useMedicationStatementRequestinstead ofMedicationStatement. Ensure that all other parts of the code that rely on these interfaces are updated to reflect this change.Please confirm that all usages of
StructuredDataMapandStructuredRequestMapare compatible with the new types. You can run this script to help identify any discrepancies:✅ Verification successful
Type Changes are Correctly Implemented
The change from
MedicationStatementtoMedicationStatementRequestinStructuredDataMapandStructuredRequestMapis correctly implemented and all usages are compatible. The types are properly used through the genericDataTypeForandRequestTypeFortypes, maintaining the correct distinction between request and response types throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to StructuredDataMap and StructuredRequestMap that might be affected. rg 'Structured(Data|Request)Map' -A 5Length of output: 1460
Script:
#!/bin/bash # Find files importing from the types file and check MedicationStatement usage rg -l "from ['\"].*Questionnaire/structured/types['\"]" # Check for any MedicationStatement (without Request) usage rg "MedicationStatement[^R]" -A 2Length of output: 9678
Script:
#!/bin/bash # Check the content of handlers.ts to verify type usage cat src/components/Questionnaire/structured/handlers.tsLength of output: 4886
src/components/Questionnaire/QuestionRenderer.tsx (2)
17-17: LGTM! Addition follows existing pattern.The addition of "medication_statement" to
FULL_WIDTH_QUESTION_TYPESis consistent with the existing pattern and aligns with the PR objectives.
Line range hint
8-9: Verify the structured question type definition.Let's ensure that "medication_statement" is properly defined in the
StructuredQuestionTypetype.✅ Verification successful
Type definition verified successfully
The "medication_statement" type is properly defined in
StructuredQuestionTypeas one of the valid values in the union type atsrc/types/questionnaire/question.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that "medication_statement" is defined in StructuredQuestionType # Search for StructuredQuestionType definition rg -A 10 "type StructuredQuestionType = " || rg -A 10 "enum StructuredQuestionType"Length of output: 84
Script:
#!/bin/bash # Search for StructuredQuestionType in TypeScript files with various patterns echo "=== Searching for StructuredQuestionType ===" rg -t typescript -t tsx "StructuredQuestionType" -A 5 echo -e "\n=== Searching for medication_statement ===" rg -t typescript -t tsx "medication_statement"Length of output: 370
Script:
#!/bin/bash # Search in .ts and .tsx files explicitly echo "=== Searching for StructuredQuestionType ===" rg "StructuredQuestionType" --glob "*.{ts,tsx}" -A 5 echo -e "\n=== Using ast-grep for type definition ===" ast-grep --pattern 'type StructuredQuestionType = $_' echo -e "\n=== Searching for medication_statement ===" rg "medication_statement" --glob "*.{ts,tsx}"Length of output: 9475
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (4)
Line range hint
174-204: Consider extracting grid column configuration.The grid layout is well-structured, but the column configuration could be extracted into a constant to improve maintainability. This would make it easier to modify column widths and ensure consistency across the component.
+const GRID_COLUMNS = { + medicine: '280px', + dosage: '180px', + frequency: '170px', + duration: '160px', + instructions: '300px', + route: '180px', + site: '250px', + method: '180px', + intent: '160px', + notes: '200px', + actions: '48px' +}; + +const GRID_TEMPLATE = Object.values(GRID_COLUMNS).join(','); + -<div className="hidden lg:grid grid-cols-[280px,180px,170px,160px,300px,180px,250px,180px,160px,200px,48px] bg-gray-50 border-b text-sm font-medium text-gray-500"> +<div className={cn("hidden lg:grid bg-gray-50 border-b text-sm font-medium text-gray-500", `grid-cols-[${GRID_TEMPLATE}]`)}>
656-696: Enhance type safety for system identifiers.The implementation is logically sound, but the system identifiers could benefit from type safety to prevent potential errors.
+const INSTRUCTION_SYSTEMS = { + PRN_REASON: 'system-as-needed-reason', + ADDITIONAL_INSTRUCTION: 'system-additional-instruction' +} as const; + {dosageInstruction?.as_needed_boolean ? ( <DualValueSetSelect systems={[ - "system-as-needed-reason", - "system-additional-instruction", + INSTRUCTION_SYSTEMS.PRN_REASON, + INSTRUCTION_SYSTEMS.ADDITIONAL_INSTRUCTION, ]}
763-765: Remove duplicate label in desktop view.The label is rendered twice in desktop view, as it's already shown in the header.
- <Label className="mb-1.5 block text-sm lg:hidden"> - {t("notes")} - </Label>
777-783: Improve questionnaireResponse structure.The questionnaireResponse object contains empty strings and arrays that could be better structured or omitted.
- questionnaireResponse={{ - question_id: "", - structured_type: "medication_request", - link_id: "", - values: [], - note: medication.note, - }} + questionnaireResponse={{ + structured_type: "medication_request", + note: medication.note, + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (1)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (1)
40-41: LGTM! Clean import organization.The new imports for DualValueSetSelect and NotesInput are properly organized and follow the existing pattern.
src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx
Outdated
Show resolved
Hide resolved
src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx
Outdated
Show resolved
Hide resolved
src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/components/Medicine/utils.ts (2)
4-17: Consider enhancing type safety and readability.The function handles dosage formatting well, but could be improved:
export function formatDosage(instruction: MedicationRequestDosageInstruction) { if (!instruction.dose_and_rate) return ""; if (instruction.dose_and_rate.type === "calculated") { const { dose_range } = instruction.dose_and_rate; if (!dose_range) return ""; - return `${dose_range.low.value}${dose_range.low.unit.display} - ${dose_range.high.value}${dose_range.high.unit.display}`; + const { low, high } = dose_range; + return `${low.value}${low.unit.display} - ${high.value}${high.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}`; }
19-38: Add JSDoc documentation and optimize the implementation.The function could benefit from documentation and a minor optimization:
+/** + * Formats dosage instructions in Rx style + * @param instruction - The dosage instruction object + * @returns Formatted string containing route, method, and site information + */ export function formatSig(instruction: MedicationRequestDosageInstruction) { const parts: string[] = []; + + // Early return if no formatting needed + if (!instruction.route?.display && !instruction.method?.display && !instruction.site?.display) { + return ""; + } // Add route if present if (instruction.route?.display) { parts.push(`Via ${instruction.route.display}`); }src/components/Questionnaire/DualValueSetSelect.tsx (2)
31-40: Add JSDoc documentation for the props interface.The props interface could benefit from detailed documentation:
+/** + * Props for the DualValueSetSelect component + * @interface DualValueSetSelectProps + * @property {[ValueSetSystem, ValueSetSystem]} systems - Array of two value set systems + * @property {[Code | null, Code | null]} [values] - Currently selected values for both systems + * @property {(index: TabIndex, value: Code | null) => void} onSelect - Callback when a value is selected + * @property {[string, string]} [placeholders] - Placeholder texts for both inputs + * @property {boolean} [disabled] - Whether the component is disabled + * @property {number} [count] - Number of items to show in the list + * @property {string} [searchPostFix] - String to append to search query + * @property {[string, string]} labels - Labels for both tabs + */ interface DualValueSetSelectProps { systems: [ValueSetSystem, ValueSetSystem]; values?: [Code | null, Code | null]; onSelect: (index: TabIndex, value: Code | null) => void; placeholders?: [string, string]; disabled?: boolean; count?: number; searchPostFix?: string; labels: [string, string]; }
92-136: Enhance keyboard accessibility.The component could benefit from improved keyboard interaction:
<Button variant="outline" role="combobox" + aria-expanded={open} + aria-haspopup="listbox" className={cn( "w-full justify-start h-auto min-h-10 py-2", "text-left", !hasValues && "text-gray-400", )} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + setOpen(true); + } + }} >src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (1)
Line range hint
242-252: Add aria-label for the expand button.The expand button already has an aria-label, but it could be more descriptive by including the medication name.
Apply this diff:
<Button - aria-label="Expand Medication Request" + aria-label={t("expand_medication_request", { medication: medication.medication?.display })} variant="ghost" size="icon" className="h-8 w-8 text-gray-500 hover:text-gray-900" onClick={(e) => { e.stopPropagation(); setExpandedMedicationIndex(index); }} disabled={disabled} > <Pencil2Icon className="h-4 w-4" /> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/locale/en.json(8 hunks)src/components/Medicine/MedicationsTable.tsx(1 hunks)src/components/Medicine/utils.ts(1 hunks)src/components/Questionnaire/DualValueSetSelect.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(11 hunks)src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/components/Medicine/MedicationsTable.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx (2)
Line range hint
1-305: LGTM! Well-structured implementation with responsive design.The component effectively handles medication statements with proper state management, event handling, and responsive layout using
Collapsiblefor mobile views and grid layout for desktop views.
309-503: LGTM! Well-structured grid row implementation with proper accessibility.The component effectively renders a medication statement row with:
- Proper type definitions using
MedicationStatementRequest- Responsive design with
NotesInputfor mobile andInputfor desktop- Accessible form controls with associated labels
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (3)
Line range hint
1-305: LGTM! Well-structured implementation with responsive design.The component effectively handles medication requests with proper state management, event handling, and responsive layout using
Collapsiblefor mobile views and grid layout for desktop views.
643-683: LGTM! Clean implementation of PRN reasons and additional instructions.The
DualValueSetSelectcomponent is effectively used to handle both PRN reasons and additional instructions in a single component, improving the user experience.
745-777: LGTM! Well-structured notes implementation with responsive design.The notes section is properly implemented with:
Inputcomponent for desktop viewNotesInputcomponent for mobile view- Proper accessibility with labels
| const searchQuery = useQuery({ | ||
| queryKey: ["valueset", systems[activeTab], "expand", count, search], | ||
| queryFn: query.debounced(routes.valueset.expand, { | ||
| pathParams: { system: systems[activeTab] }, | ||
| body: { count, search: search + searchPostFix }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the search query.
The search query implementation should handle errors:
const searchQuery = useQuery({
queryKey: ["valueset", systems[activeTab], "expand", count, search],
queryFn: query.debounced(routes.valueset.expand, {
pathParams: { system: systems[activeTab] },
body: { count, search: search + searchPostFix },
}),
+ retry: 2,
+ onError: (error) => {
+ console.error('Failed to fetch value set:', error);
+ // Consider showing a user-friendly error message
+ }
});📝 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 searchQuery = useQuery({ | |
| queryKey: ["valueset", systems[activeTab], "expand", count, search], | |
| queryFn: query.debounced(routes.valueset.expand, { | |
| pathParams: { system: systems[activeTab] }, | |
| body: { count, search: search + searchPostFix }, | |
| }), | |
| }); | |
| const searchQuery = useQuery({ | |
| queryKey: ["valueset", systems[activeTab], "expand", count, search], | |
| queryFn: query.debounced(routes.valueset.expand, { | |
| pathParams: { system: systems[activeTab] }, | |
| body: { count, search: search + searchPostFix }, | |
| }), | |
| retry: 2, | |
| onError: (error) => { | |
| console.error('Failed to fetch value set:', error); | |
| // Consider showing a user-friendly error message | |
| } | |
| }); |
| {/* Remove Button */} | ||
| <div className="hidden lg:flex lg:px-2 lg:py-1 items-center justify-center sticky right-0 bg-white shadow-[-12px_0_15px_-4px_rgba(0,0,0,0.15)] w-12"> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={onRemove} | ||
| disabled={disabled} | ||
| className="h-8 w-8" | ||
| > | ||
| <MinusCircledIcon className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-label for the remove button.
The remove button is missing an aria-label for accessibility.
Apply this diff:
<Button
variant="ghost"
size="icon"
+ aria-label={t("remove_medication")}
onClick={onRemove}
disabled={disabled}
className="h-8 w-8"
>
<MinusCircledIcon className="h-4 w-4" />
</Button>Committable suggestion skipped: line range outside the PR's diff.
src/types/emr/medicationStatement.ts
Outdated
| export type MedicationStatementRequest = { | ||
| status: MedicationStatementStatus; | ||
| reason?: string; | ||
| medication: Code; | ||
| dosage_text?: string; | ||
| effective_period?: Period; | ||
| information_source?: MedicationStatementInformationSourceType; | ||
| note?: string; | ||
| }; |
There was a problem hiding this comment.
Too many optional fields, why don't we make the information_source mandatory? also the dosage_text 🤔
| @@ -0,0 +1,185 @@ | |||
| import { useQuery } from "@tanstack/react-query"; | |||
There was a problem hiding this comment.
I am not sure why we need DualValueSetSelect it!
|
Reminder: To add the "needs testing" label, comment "ready for testing" on this PR. |
…dual-valueset-field-medication
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/components/Questionnaire/ValueSetSelect.tsx (2)
35-36: LGTM! Consider adding JSDoc comments.The new props are well-typed and their names clearly indicate their purpose. Consider adding JSDoc comments to document their usage and impact on component behavior.
+ /** Hide the trigger button and render content directly */ hideTrigger?: boolean; + /** Control the popover's open state externally */ controlledOpen?: boolean;
107-135: Consider adding ARIA label for accessibility.The conditional rendering logic is clean and maintainable. Consider adding an aria-label to improve accessibility when the trigger is hidden.
<Command filter={() => 1}> <CommandInput + aria-label={placeholder} placeholder={placeholder} className="outline-none border-none ring-0 shadow-none" onValueChange={setSearch} />src/components/Medicine/DualValueSetSelect.tsx (3)
19-30: LGTM! Consider enhancing type safety for labels prop.The types are well-defined. Consider making the labels prop type more specific to ensure meaningful labels.
- labels: [string, string]; + type TabLabel = 'instructions' | 'reasons' | string; + labels: [TabLabel, TabLabel];
56-101: Consider memoizing the mapped values for performance.The trigger button implementation is clean and accessible. For better performance with large datasets, consider memoizing the mapped values.
+ const renderedValues = useMemo( + () => values.map( (value, index) => value && ( <div key={value.code}> {/* existing implementation */} </div> ), ), + [values, onSelect] + );
103-116: Consider using numeric tab values for cleaner type handling.The tabs implementation works well, but the string-number conversion could be handled more elegantly.
- value={activeTab.toString()} - onValueChange={(value) => { - setActiveTab(Number(value) as TabIndex); - }} + value={activeTab} + onValueChange={(value: TabIndex) => { + setActiveTab(value); + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(8 hunks)src/components/Medicine/DualValueSetSelect.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(11 hunks)src/components/Questionnaire/ValueSetSelect.tsx(3 hunks)src/types/emr/medicationStatement.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (7)
src/types/emr/medicationStatement.ts (3)
32-32: Verify the impact of making fields required.Good decision to make
dosage_textandinformation_sourcerequired fields as suggested in the previous review. However, this is a breaking change that needs verification.Let's verify all usages to ensure they provide these now-required fields:
#!/bin/bash # Search for MedicationStatement object creations/assignments ast-grep --pattern 'MedicationStatement = { $$$ }' # Also check for spread operations that might miss these fields rg -A 5 'MedicationStatement.*=.*\{.*\.\.\.'Also applies to: 38-38
43-51: Well-structured request type!Good separation of concerns by creating a dedicated request type that omits server-side fields (
id,patient,encounter). The consistent field ordering withMedicationStatementimproves maintainability.
58-58: LGTM! Consistent type definitions.The changes to make
dosage_textandinformation_sourcerequired are consistently applied across bothMedicationStatementandMedicationStatementReadtypes.Also applies to: 61-61
src/components/Questionnaire/ValueSetSelect.tsx (2)
52-52: LGTM! Clean state management implementation.The state management cleanly handles both controlled and uncontrolled modes, maintaining the search reset behavior.
Also applies to: 64-67
69-105: LGTM! Good extraction of reusable content.The Command component logic is cleanly extracted and reused, following DRY principles while maintaining UI consistency.
src/components/Medicine/DualValueSetSelect.tsx (2)
42-51: LGTM! Clean state management implementation.The state management and event handling are well-implemented, with good UX considerations for tab reset.
118-136: LGTM! Clean integration with ValueSetSelect.The ValueSetSelect integration is well-implemented, making good use of the new features for a seamless dual-selection experience.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/Medicine/MultiValueSetSelect.tsx (2)
74-78: Reconsider event propagation handling.Using
stopPropagationmight prevent event bubbling needed by parent components. Consider using a more specific approach to handle click events.-onClick={(e) => { - e.stopPropagation(); +onClick={() => { setActiveTab(index); setOpen(true); }}
127-134: Consider making tab auto-advance optional.The automatic tab advancement after selection might be confusing for users who want to make multiple selections in the same category. Consider making this behavior configurable.
+interface MultiValueSetSelectProps { + autoAdvanceTabs?: boolean; + // ... existing props +} onSelect={(value) => { options[activeTab].onSelect(value); - if (activeTab < options.length - 1) { + if (autoAdvanceTabs && activeTab < options.length - 1) { setActiveTab(activeTab + 1); } else { setOpen(false); } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Medicine/MultiValueSetSelect.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/components/Medicine/MultiValueSetSelect.tsx (1)
19-25: Consider adding runtime validation for nullable values.The
valuefield inValueSetOptionis nullable (Code | null). While TypeScript provides compile-time type safety, consider adding runtime validation to ensure proper handling of null values, especially since this affects the UI state.Let's verify how null values are handled across the codebase:
| return ( | ||
| <div className="w-full"> | ||
| <Popover open={open} onOpenChange={handleOpenChange}> | ||
| <PopoverTrigger asChild> | ||
| <Button | ||
| variant="outline" | ||
| role="combobox" | ||
| className={cn( | ||
| "w-full justify-start h-auto min-h-10 py-2", | ||
| "text-left", | ||
| !hasValues && "text-gray-400", | ||
| )} | ||
| > | ||
| {!hasValues ? ( | ||
| <span>{options[0]?.placeholder || t("search")}</span> | ||
| ) : ( | ||
| <div className="flex flex-col gap-1 w-full"> | ||
| {options.map( | ||
| (option, index) => | ||
| option.value && ( | ||
| <div | ||
| key={option.value.code} | ||
| className="flex items-center justify-between bg-gray-100 rounded px-2 py-1 min-w-0" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setActiveTab(index); | ||
| setOpen(true); | ||
| }} | ||
| > | ||
| <span className="text-sm truncate flex-1 mr-2"> | ||
| {option.value.display} | ||
| </span> | ||
| <Button | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| option.onSelect(null); | ||
| }} | ||
| variant="ghost" | ||
| size="icon" | ||
| > | ||
| <X className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| ), | ||
| )} | ||
| </div> | ||
| )} | ||
| </Button> | ||
| </PopoverTrigger> | ||
| <PopoverContent | ||
| className="p-0 inline-block min-w-[300px]" | ||
| align="start" | ||
| sideOffset={4} | ||
| > | ||
| <Tabs | ||
| value={activeTab.toString()} | ||
| onValueChange={(value) => { | ||
| setActiveTab(Number(value)); | ||
| }} | ||
| className="w-fit" | ||
| > | ||
| <TabsList className="flex"> | ||
| {options.map((option, index) => ( | ||
| <TabsTrigger | ||
| key={index} | ||
| value={index.toString()} | ||
| className="whitespace-nowrap px-3" | ||
| > | ||
| {option.label} | ||
| </TabsTrigger> | ||
| ))} | ||
| </TabsList> | ||
| <div className="w-full"> | ||
| <ValueSetSelect | ||
| system={options[activeTab].system} | ||
| value={options[activeTab].value} | ||
| onSelect={(value) => { | ||
| options[activeTab].onSelect(value); | ||
| if (activeTab < options.length - 1) { | ||
| setActiveTab(activeTab + 1); | ||
| } else { | ||
| setOpen(false); | ||
| } | ||
| }} | ||
| placeholder={options[activeTab].placeholder || t("search")} | ||
| disabled={disabled} | ||
| count={count} | ||
| searchPostFix={searchPostFix} | ||
| wrapTextForSmallScreen | ||
| hideTrigger | ||
| controlledOpen={open} | ||
| /> | ||
| </div> | ||
| </Tabs> | ||
| </PopoverContent> | ||
| </Popover> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard navigation support.
The component lacks keyboard navigation support. Users should be able to:
- Navigate between tabs using arrow keys
- Open/close popover using Enter/Space
- Select/deselect values using Enter/Space
Would you like me to provide the implementation for keyboard navigation support?
| const [activeTab, setActiveTab] = useState<number>(0); | ||
| const [open, setOpen] = useState(false); | ||
| const hasValues = options.some((opt) => opt.value !== null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add effect to sync activeTab with options array.
The activeTab state could get out of sync if the options array changes. Consider adding a useEffect hook to reset or adjust the activeTab when options change.
+import { useEffect } from "react";
+
const [activeTab, setActiveTab] = useState<number>(0);
const [open, setOpen] = useState(false);
const hasValues = options.some((opt) => opt.value !== null);
+
+useEffect(() => {
+ if (activeTab >= options.length) {
+ setActiveTab(0);
+ }
+}, [options.length, activeTab]);📝 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 [activeTab, setActiveTab] = useState<number>(0); | |
| const [open, setOpen] = useState(false); | |
| const hasValues = options.some((opt) => opt.value !== null); | |
| import { useEffect } from "react"; | |
| const [activeTab, setActiveTab] = useState<number>(0); | |
| const [open, setOpen] = useState(false); | |
| const hasValues = options.some((opt) => opt.value !== null); | |
| useEffect(() => { | |
| if (activeTab >= options.length) { | |
| setActiveTab(0); | |
| } | |
| }, [options.length, activeTab]); |
| <Button | ||
| variant="outline" | ||
| role="combobox" | ||
| className={cn( | ||
| "w-full justify-start h-auto min-h-10 py-2", | ||
| "text-left", | ||
| !hasValues && "text-gray-400", | ||
| )} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility by using correct ARIA attributes.
The combobox role might not be appropriate for this multi-select implementation. Consider using listbox with aria-multiselectable="true" instead. Also, add necessary ARIA labels for better screen reader support.
<Button
variant="outline"
- role="combobox"
+ role="listbox"
+ aria-multiselectable="true"
+ aria-label={t("select_medication_instructions")}
className={cn(
"w-full justify-start h-auto min-h-10 py-2",
"text-left",
!hasValues && "text-gray-400",
)}
>📝 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.
| <Button | |
| variant="outline" | |
| role="combobox" | |
| className={cn( | |
| "w-full justify-start h-auto min-h-10 py-2", | |
| "text-left", | |
| !hasValues && "text-gray-400", | |
| )} | |
| > | |
| <Button | |
| variant="outline" | |
| role="listbox" | |
| aria-multiselectable="true" | |
| aria-label={t("select_medication_instructions")} | |
| className={cn( | |
| "w-full justify-start h-auto min-h-10 py-2", | |
| "text-left", | |
| !hasValues && "text-gray-400", | |
| )} | |
| > |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
MedicationsTablecomponent for displaying medication detailsImprovements
Localization