-
Notifications
You must be signed in to change notification settings - Fork 470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Discharge patient showing live #9415
base: develop
Are you sure you want to change the base?
Fix: Discharge patient showing live #9415
Conversation
WalkthroughThe pull request focuses on addressing patient status display inconsistencies across two components: Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/ConsultationCard.tsx (3)
Line range hint
18-22
: Consider renaming isLastConsultation prop for clarityThe
isLastConsultation
prop is being used to determine if a patient is discharged, but its name doesn't clearly convey this purpose. This could lead to confusion and maintenance issues.Consider renaming it to something more explicit like
isDischargedConsultation
or adding a JSDoc comment to clarify its purpose:interface ConsultationProps { itemData: ConsultationModel; - isLastConsultation?: boolean; + /** Indicates if this consultation resulted in patient discharge */ + isDischargedConsultation?: boolean; refetch: () => void; }
57-72
: Simplify the layout structureThe current layout uses nested flex containers with translation transforms, which makes it harder to maintain and could cause layout issues across different screen sizes.
Consider simplifying the layout:
-<div className="flex flex-wrap items-center gap-4 mt-4"> - <div className="flex flex-col gap-1"> +<div className="grid grid-cols-1 sm:grid-cols-2 gap-4 mt-4"> + <div className="flex items-center gap-4"> <Chip size="small" variant={itemData.suggestion === "A" ? "alert" : "primary"} text={ itemData.suggestion === "A" ? t("ip_encounter") : t("op_encounter") } - className="-translate-y-2" /> - <div className="text-sm"> + <div className="text-sm whitespace-nowrap"> {dayjs(itemData.created_date).format("DD/MM/YYYY")} </div> </div>
83-88
: Reconsider the implementation approach for patient statusThe current implementation relies on
isLastConsultation
to determine patient status, but this might not be the most reliable indicator. TheitemData.discharge_date
would be a more direct and reliable way to determine if a patient is discharged.Consider these points:
- A patient's discharge status should be determined by the presence of
discharge_date
- The
isLastConsultation
prop might not always correlate with discharge status- This change should be coordinated with the removal of status display from
Demography.tsx
to ensure consistent status display across the application
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/ConsultationCard.tsx
(2 hunks)src/components/Patient/PatientDetailsTab/Demography.tsx
(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/Patient/PatientDetailsTab/Demography.tsx
🔇 Additional comments (1)
src/components/Facility/ConsultationCard.tsx (1)
74-89
:
Multiple improvements needed in the patient status section
- The status text "LIVE" and "DISCHARGED" should use i18n translations
- The negative translate value (-translate-y-5) suggests a layout issue
- The status logic might be incorrect based on the PR title "Fix: Discharge patient showing live"
Here's the suggested fix:
{/* Patient Status Section */}
-<div className="flex items-center gap-2 -translate-y-5 ">
+<div className="flex items-center gap-2">
<div className="text-sm font-normal leading-5">
{t("patient_status")}
</div>
<Chip
size="medium"
variant="custom"
className={
- !isLastConsultation
+ itemData.discharge_date
? "bg-red-100 text-red-800"
: "bg-blue-100 text-blue-800"
}
- text={!isLastConsultation ? "LIVE" : "DISCHARGED"}
+ text={itemData.discharge_date ? t("discharged") : t("live")}
/>
</div>
Let's verify the discharge status logic across the codebase:
✅ Verification successful
The review comment suggestions are correct and should be implemented
The codebase analysis confirms:
-
The hardcoded "LIVE" and "DISCHARGED" strings should use translations as they are already available in multiple language files (en.json, kn.json, ta.json, etc.) with keys "live" and "discharged"
-
The -translate-y-5 appears to be an arbitrary value not used elsewhere in similar contexts, suggesting it's a hacky fix
-
The logic for determining status should indeed be based on discharge_date as suggested in the review, as this is the consistent pattern used throughout the codebase for determining patient discharge status (seen in multiple components like PatientHome.tsx, PatientInfoCard.tsx, etc.)
The suggested fix in the review comment is the correct approach:
- Uses proper i18n translations
- Removes the hacky translation
- Uses the correct discharge_date based logic which aligns with the rest of the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that determine patient discharge status
rg -A 5 "discharge_date|isLastConsultation" --type tsx
Length of output: 84
Script:
#!/bin/bash
# Search for discharge status related code with proper file extensions
rg -t ts -t tsx -A 5 "discharge_date|isLastConsultation"
# Also search for translation keys related to patient status
rg -t ts -t tsx -t json "\"live\"|\"discharged\"|\"patient_status\""
Length of output: 179
Script:
#!/bin/bash
# Search for discharge status related code
rg -g "*.{ts,tsx,js,jsx}" "discharge_date|isLastConsultation" -A 5
# Search for translation keys related to patient status
rg -g "*.{ts,tsx,js,jsx,json}" "\"live\"|\"discharged\"|\"patient_status\""
Length of output: 44391
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style