Cleanup #9627
Cleanup #9627
Conversation
WalkthroughThis pull request introduces several modifications across multiple components in the medical record system. The changes primarily focus on improving dialog management in medicine administration components, enhancing type safety, and refactoring code structure. Key updates include the addition of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
Deploying care-fe with
|
| Latest commit: |
6153dcd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://81867d60.care-fe.pages.dev |
| Branch Preview URL: | https://cleanup.care-fe.pages.dev |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Reminder: To add the "needs testing" label, comment "ready for testing" on this PR. |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
cleanup
|
| Run status |
|
| Run duration | 01m 08s |
| Commit |
|
| Committer | Khavin Shankar |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
3
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/components/Patient/diagnosis/list.tsx (1)
124-126: Provide a fallback for missing user data.Using optional chaining prevents runtime errors if
created_byis undefined. However, consider showing a default avatar or a placeholder name ifcreated_byis not available—this enhances the user experience in cases where creator info is missing.Also applies to: 129-130
src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx (2)
112-130: Improved rendering logic
Switching from a table to a list approach is more flexible and easier to maintain. This layout will likely be more responsive and simpler to style.
141-262: Consider time zones foronset_datetime
Storing date/time usingtoISOString()ensures a consistent UTC timestamp. However, if local times are required for display or if offset is relevant, consider surfacing time zone logic.src/components/Questionnaire/QuestionTypes/AllergyQuestion.tsx (1)
151-330: Consider verifying time zone context forlast_occurrence
Similar to the diagnosis data, if local time is critical, you may need a clearer offset or time zone approach.src/components/ui/button.tsx (1)
8-8: Addedmax-w-full truncateclasses
This ensures the button doesn’t overflow its container and handles text overflow gracefully. If vital information might get truncated, consider adding tooltips.src/components/Medicine/DiscontinueMedication.tsx (1)
162-171: Wrapping buttons inDialogClose
This ensures the dialog closes by default when pressing these buttons. If asynchronous tasks need to finish first, consider handling the close logic after successful completion.src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx (1)
158-158: Enhance readability of class concatenations.Consider grouping similar classes together to highlight their overall purpose (e.g. layout vs. spacing vs. text styling). This minor improvement can make future modifications clearer.
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (1)
152-152: Improve clarity of medication heading.The heading is nicely spaced and wrapped. However, consider visually separating the medication name from the index (e.g., adding a colon or limiting the line break) for better readability.
src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx (2)
40-48: Ensure date correctness and user intent for default onset date
By settingonset_datetime: new Date().toISOString(), all new symptoms start at the current date/time, which might not always reflect the actual onset date. If you intended to track precise onset times or allow users to set backdated dates, consider defaulting this field toundefinedor prompting users for the date.
Line range hint
62-67: Consider preventing duplicate symptom entries
If the user attempts to add the same symptom code multiple times, you may want to either merge them or alert the user. Otherwise, the list might contain identical codes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/components/Medicine/AdministerMedicine.tsx(3 hunks)src/components/Medicine/DiscontinueMedication.tsx(3 hunks)src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx(1 hunks)src/components/Patient/diagnosis/list.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/AllergyQuestion.tsx(4 hunks)src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx(3 hunks)src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(3 hunks)src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx(4 hunks)src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx(3 hunks)src/components/Questionnaire/QuestionnaireForm.tsx(1 hunks)src/components/Questionnaire/QuestionnaireSearch.tsx(4 hunks)src/components/ui/button.tsx(1 hunks)src/types/emr/allergyIntolerance.ts(2 hunks)src/types/questionnaire/diagnosis.ts(1 hunks)src/types/questionnaire/symptom.ts(1 hunks)
🔇 Additional comments (33)
src/components/Questionnaire/QuestionnaireSearch.tsx (4)
2-2: Use of React imports is appropriate here.
This import line keeps the code concise by only bringing useState and useMemo, which appear to be effectively used.
20-20: Optional selected prop is a good addition.
This optional list of selected questionnaires prevents potential crashes or undefined references when the prop isn’t passed, improving component safety.
32-32: Destructuring the selected property makes the component clearer.
Directly unpacking selected from props is a clean approach to access it in the component.
47-56: Efficient two-step filtering with memoization.
- First filter excludes already selected questionnaires, preventing duplicates in the search results.
- Second filter applies the search term, ensuring only relevant items are displayed.
The useMemo hook ensures that the filters recompute only when questionnaires, search, or selected changes, helping performance. This is an excellent approach for large datasets.
src/components/Questionnaire/QuestionnaireForm.tsx (1)
381-381: Passing the selected prop ensures the QuestionnaireSearch can exclude existing forms.
This elegantly prevents duplication by mapping over questionnaireForms to provide the component with already-selected questionnaires, binding the logic together with minimal overhead.
src/types/questionnaire/diagnosis.ts (1)
42-43: Consider extending separate types instead of making the fields optional.
You can follow your previously discussed approach (e.g. CreateDiagnosis vs. EditDiagnosis) if the created_by and updated_by fields are not always present. Making them optional in the base interface can introduce ambiguity and complicate validation for all diagnosis objects.
src/types/questionnaire/symptom.ts (1)
46-47: Same concern regarding optionality of fields.
Following the same approach as in Diagnosis, consider extending separate interfaces instead of making created_by and updated_by optional in the base Symptom interface.
src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx (3)
1-3: Imports updated for icons and translation
These changes correctly import the needed icons and the useTranslation hook. No issues found.
59-60: Validate code before adding a new diagnosis
When adding a new diagnosis, ensure that code is valid. A null or undefined code might cause unintended behavior. Consider adding guards or user feedback if code is invalid.
87-90: Logic for updating diagnosis
The spread operator usage is correct, ensuring partial updates to the existing diagnosis object. This is a good approach for maintainability.
src/components/Questionnaire/QuestionTypes/AllergyQuestion.tsx (5)
1-3: Imports updated for icons and translation
Similar to the DiagnosisQuestion component, these imports improve readability and localization. No issues.
42-52: Default values for ALLERGY_INITIAL_VALUE
Defining defaults keeps new allergy items consistent. Ensure these defaults match typical user expectations (e.g., clinical_status = "active").
68-72: Handle code validity for new allergies
Similar to the DiagnosisQuestion component, make sure code is valid to avoid undefined references.
Line range hint 99-105: Partial updates for allergy
The usage of partial updates is consistent and ensures that existing properties aren’t overwritten inadvertently.
124-140: Dynamic list rendering
Replacing table rows with items in a list yields a more responsive UI. This approach is consistent with other components.
src/types/emr/allergyIntolerance.ts (3)
4-41: Improved type safety
These constants and corresponding type definitions (AllergyIntoleranceClinicalStatus, etc.) significantly enhance clarity and maintainability by enumerating valid status values.
46-49: Optional properties enforce best practices
Marking clinical_status, verification_status, category, and criticality as optional is consistent with real-world scenarios. Ensure to handle undefined cases in the UI.
59-62: Stricter interface for request payload
Requiring these fields encourages consistent request data. This helps prevent errors when calling APIs.
src/components/Medicine/DiscontinueMedication.tsx (1)
31-31: Use of DialogClose
The new DialogClose import is properly integrated, allowing the dialog to close when the wrapped elements are clicked.
src/components/Questionnaire/QuestionTypes/MedicationStatementQuestion.tsx (3)
257-257: No issues found.
These added class names enhance responsive design by preventing undesired horizontal overflow. Implementation looks good.
309-309: Check if negative margins are necessary.
You're already using flex-wrap and gap. Verify whether you need additional margin or padding adjustments to avoid any layout breaks, especially on smaller screen widths.
332-332: Good alignment approach for button label.
The use of justify-start in conjunction with flex gap-1.5 items-center offers clarity and consistent alignment for the button contents. Looks good.
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (2)
195-195: Appropriate usage of flex-wrap and gap.
This maintains responsiveness for dosage information on smaller screens. No issues here.
283-283: Ensure consistent display logic for PRN vs. scheduled medication.
When rendering this block for scheduled medications, re-check that the flex wrapping doesn't push essential form fields out of view on small screens. A quick test on mobile would be helpful.
src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx (1)
89-108: Dialog component usage is consistent and well-structured.
Your approach conditionally renders the medication discontinuation and administration actions unless in read-only mode. This is coherent with best practices. Make sure to confirm that the rest of the code using these dialogs is in sync with the new UI patterns (especially the re-fetch logic after success).
src/components/Medicine/AdministerMedicine.tsx (2)
37-37: Introduction of DialogClose is appropriate.
Using DialogClose helps keep the Dialog state consistent, preventing accidental leftover open states. No issues at this import statement.
289-298: Well-handled asynchronous flow and dialog closing.
- The usage of “asChild” ensures the button triggers dialog closure.
- The asynchronous mutation uses await, which is good for clarity in flow control.
Everything looks properly coordinated.
src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx (6)
Line range hint 90-108: Deep merging for nested fields
When updating nested fields like onset, consider whether you want to preserve or merge other keys within the onset object. The current pattern overwrites the entire onset object when updating. This might be intentional but is worth confirming.
112-126: Great transition to a list-based layout
Switching from table layouts to lists is more mobile-friendly and accessible. This implementation is clear and maintainable.
129-134: Validate integration of ValueSetSelect
The usage of ValueSetSelect is straightforward. Ensure that external components or data sources calling handleAddSymptom() handle potential errors or edge cases (e.g., network latency, empty results).
259-285: Confirm handling for empty and undefined notes
Right now, the Textarea is displayed only when symptom.note !== undefined. A note of null or other falsey values might not trigger it. If you want to hide only when the note is strictly undefined (uninitialized), this is fine. Otherwise, check for other edge cases (null, empty string).
1-3: Overall well-structured component
Your changes effectively modularize the symptom details via SymptomItem, improving clarity and maintainability. The overall approach to translation, form updates, and reusability is good. Great job!
Also applies to: 139-291
152-153: 🛠️ Refactor suggestion
Avoid using array indices as the key
Using index as the key in <li key={index}> can introduce re-render glitches if symptoms are reordered or removed. Consider using a stable unique identifier like symptom.code.id or a generated UUID.
- <li key={index}>
+ <li key={symptom.code.id || index}>Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx (2)
188-195: Add aria-label to icon buttonThe remove button with MinusCircledIcon should have an aria-label for better accessibility.
<Button variant="secondary" size="icon" onClick={onRemove} disabled={disabled} + aria-label={t("remove_diagnosis")} > <MinusCircledIcon className="size-4" /> </Button>
147-153: Consider adding error boundaryThe DiagnosisItem component handles complex state and UI rendering. Consider wrapping it with an error boundary to gracefully handle rendering errors.
class DiagnosisItemErrorBoundary extends React.Component { // ... error boundary implementation } // Usage: <DiagnosisItemErrorBoundary> <DiagnosisItem {...props} /> </DiagnosisItemErrorBoundary>src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx (1)
135-140: Consider caching ValueSetSelect resultsThe ValueSetSelect component might be making API calls to fetch condition codes. Consider caching the results to improve performance.
// Example implementation using React Query const { data: conditionCodes } = useQuery(['condition-codes'], () => fetchConditionCodes('system-condition-code'), { staleTime: 5 * 60 * 1000, // Consider results fresh for 5 minutes cacheTime: 30 * 60 * 1000 // Keep in cache for 30 minutes } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx(3 hunks)src/components/Questionnaire/QuestionTypes/SymptomQuestion.tsx(3 hunks)
🔇 Additional comments (4)
src/components/Questionnaire/QuestionTypes/DiagnosisQuestion.tsx (4)
32-32: Great improvement in type safety!
Replacing any with the Question type enhances type checking and code maintainability.
Line range hint 62-92: Well-structured state management!
The handlers maintain immutability and properly update both local state and parent component through callbacks.
119-134: Excellent mobile-responsive implementation!
The switch from table to list layout with proper semantic HTML (ul/li) improves mobile responsiveness. The space-y-4 and divide-y-2 classes provide consistent spacing and visual separation.
44-46: Consider timezone handling for onset_datetime
Using new Date().toISOString() as the default value might lead to timezone-related issues. Consider using a UTC-aware approach or allowing the client's timezone to be explicitly specified.
|
Reminder: To add the "needs testing" label, comment "ready for testing" on this PR. |
| @@ -121,13 +121,13 @@ export function DiagnosisList({ patientId, encounterId }: DiagnosisListProps) { | |||
| </TableCell> | |||
| <TableCell className="whitespace-nowrap flex items-center gap-2"> | |||
| <Avatar | |||
| name={`${diagnosis.created_by.first_name} ${diagnosis.created_by.last_name}`} | |||
| name={`${diagnosis.created_by?.first_name} ${diagnosis.created_by?.last_name}`} | |||
There was a problem hiding this comment.
i don't see any reason for this null check. backend always provides this right? and we do have separate types for request and response.
| value={status} | ||
| className="capitalize" | ||
| > | ||
| {status.replace(/_/g, " ")} |
There was a problem hiding this comment.
i'd recommend adding this to translations instead
| </Button> | ||
| </DialogClose> | ||
| <DialogClose asChild> | ||
| <Button type="submit" className="bg-primary"> |
There was a problem hiding this comment.
| <Button type="submit" className="bg-primary"> | |
| <Button type="submit" variant="primary"> |
| </Button> | ||
| </DialogClose> | ||
| <DialogClose asChild> | ||
| <Button type="submit" className="bg-danger-500"> |
There was a problem hiding this comment.
use variants :)
| <Button type="submit" className="bg-danger-500"> | |
| <Button type="submit" variant="destructive"> |
| prescription={prescription} | ||
| onClose={(success) => { | ||
| if (success) { | ||
| props.refetch(); |
There was a problem hiding this comment.
i'd recommend using query invalidation instead of passing refetch as that's the newer approach with tanstack query we follow and automatically handles invalidations of queries in multiple places if used in multiple places
| className="flex gap-1.5 items-center justify-start" | ||
| > | ||
| <TextAlignLeftIcon className="size-4" /> | ||
| {t("Note")} |
There was a problem hiding this comment.
| {t("Note")} | |
| {t("note")} |
|
👋 Hi, @khavinshankar, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@khavinshankar status here? |
|
@bodhish not sure if this is relevant anymore, just confirming
|
|
Table like that breaks to card on mobile; https://www.figma.com/design/Z93EYKSa1MdBmXndsMBJSQ/Care?node-id=5747-713 |
|
Will create a new PR, this is not viable anymore |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
New Features
DialogClosecomponent in medicine administration and discontinuation screens.QuestionnaireSearchcomponent to reflect selected questionnaires.Bug Fixes
Style
Refactor