-
Notifications
You must be signed in to change notification settings - Fork 489
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
Remove redundant "Add Consultation" button #9095
Remove redundant "Add Consultation" button #9095
Conversation
WalkthroughThe changes in the pull request focus on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@Srayash has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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 (1)
src/components/Patient/PatientHome.tsx (1)
246-246
: Simplify the boolean expression.The boolean expression can be simplified by directly returning the value.
- return patientData.is_active ? true : false; + return patientData.is_active;🧰 Tools
🪛 Biome
[error] 246-246: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/PatientHome.tsx
(2 hunks)
🧰 Additional context used
🪛 Biome
src/components/Patient/PatientHome.tsx
[error] 246-246: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (1)
src/components/Patient/PatientHome.tsx (1)
1065-1065
: LGTM! The UI correctly implements the button visibility.
The implementation properly hides the "Add Consultation" button when the patient is inactive, which aligns with the PR objective.
Ready for Review @rithviknishad. |
another approach can be creating a separate variable isAddConsultationVisible which returns a bool value based on whether the patient is active or not and then using that instead of isPatientEligibleForNewConsultation. |
🤔 |
previous changes altered the function, there were some additional checks that were removed. Original Function: care_fe/src/components/Patient/PatientHome.tsx Lines 245 to 251 in 7acc8be
|
…onsultationButton
Completely remove Add Consultation from Patient details.
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 (2)
src/components/Patient/PatientHome.tsx (2)
244-252
: Remove commented out code.Instead of keeping the commented out function, it should be removed entirely. If needed, the code history can be referenced through version control.
- // Commented out as this is no longer used should I completely remove this? - - // const isPatientEligibleForNewConsultation = (patientData: PatientModel) => { - // return patientData.is_active && - // (!patientData?.last_consultation || - // patientData?.last_consultation?.discharge_date) - // ? true - // : false; - // };
Line range hint
1-1
: Consider component refactoring for better maintainability.The component handles multiple responsibilities and contains duplicated logic between desktop and mobile views. Consider:
- Extracting the action buttons into a separate component to maintain consistency between desktop and mobile views
- Breaking down the large component into smaller, focused components (e.g., PatientInfo, MedicalHistory, ConsultationHistory)
- Moving the visibility logic into a custom hook for reusability
Example structure:
// hooks/usePatientActions.ts const usePatientActions = (patientData: PatientModel) => { const isButtonVisible = !patientData.is_active; // ... other action-related logic return { isButtonVisible, ... }; }; // components/PatientActions.tsx const PatientActions = ({ patient, isMobile }) => { const { isButtonVisible } = usePatientActions(patient); return ( // Render buttons based on visibility logic ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/PatientHome.tsx
(1 hunks)
🔇 Additional comments (1)
src/components/Patient/PatientHome.tsx (1)
Line range hint 1-1
: Verify button visibility logic matches requirements.
The current implementation in isPatientInactive
checks both the active status AND facility association:
return !patientData.is_active || !(patientData?.last_consultation?.facility === facilityId)
However, according to the PR objectives, the button should only be hidden when the patient is inactive. The additional facility check might hide the button in cases where it should be visible.
Consider simplifying the logic to match the requirements:
-return !patientData.is_active || !(patientData?.last_consultation?.facility === facilityId)
+return !patientData.is_active
Let's verify the impact of this change:
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.
You'll also need to remove the button from mobile view
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.
LGTM, tests are failing, do have a look
The failing test wasn't related to my PR, although it has been resolved now. |
@Srayash 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! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
UI Improvements