Add View Consultation Button #9296
Conversation
WalkthroughThis pull request introduces a new "View Latest Encounter" button for patient details, enhancing the user interface for patient management. The changes include updating the English localization file, modifying the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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. |
![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
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/PatientHome.tsx (1)
242-283: Consider simplifying the consultation button rendering logicThe current implementation correctly handles the display logic for Add/View consultation buttons, but the nested conditional structure could be simplified for better readability.
Consider refactoring to this cleaner approach:
- {patientData?.is_active && - (!patientData?.last_consultation || - patientData?.last_consultation?.discharge_date) ? ( - <div> - <ButtonV2 - className="w-full" - size="default" - onClick={() => - navigate( - `/facility/${patientData?.facility}/patient/${id}/consultation`, - ) - } - > - <span className="flex w-full items-center justify-start gap-2"> - <CareIcon - icon="l-chat-bubble-user" - className="text-xl" - /> - {t("add_consultation")} - </span> - </ButtonV2> - </div> - ) : ( - <div> - <ButtonV2 - className="w-full" - size="default" - onClick={() => - navigate( - `/facility/${patientData.facility}/patient/${id}/consultation/${patientData.last_consultation?.id}/consent-records`, - ) - } - > - <span className="flex w-full items-center justify-start gap-2"> - <CareIcon - icon="l-chat-bubble-user" - className="text-xl" - /> - {t("view_consultation")} - </span> - </ButtonV2> - </div> - )} + const showAddConsultation = patientData?.is_active && + (!patientData?.last_consultation || patientData?.last_consultation?.discharge_date); + + <ButtonV2 + className="w-full" + size="default" + onClick={() => + navigate( + showAddConsultation + ? `/facility/${patientData?.facility}/patient/${id}/consultation` + : `/facility/${patientData.facility}/patient/${id}/consultation/${patientData.last_consultation?.id}/consent-records` + ) + } + > + <span className="flex w-full items-center justify-start gap-2"> + <CareIcon + icon="l-chat-bubble-user" + className="text-xl" + /> + {t(showAddConsultation ? "add_consultation" : "view_consultation")} + </span> + </ButtonV2>This refactoring:
- Extracts the condition into a descriptive variable
- Removes redundant div wrappers
- Combines the buttons into a single component with conditional props
- Reduces code duplication
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/components/Patient/PatientHome.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🔇 Additional comments (2)
src/components/Patient/PatientHome.tsx (2)
242-283: LGTM: Patient transfer button implementation
The transfer button implementation correctly handles:
- Authorization checks
- Disable conditions based on consultation and patient status
- Visual feedback with appropriate icons
- State updates after transfer status change
279-279: Verify translation key exists
Ensure that the "view_consultation" translation key is properly defined in the localization files.
✅ Verification successful
Translation key "view_consultation" is properly defined
The key exists in public/locale/en.json with the value "View Latest Encounter".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the translation key exists in the English locale file
rg -F '"view_consultation"' public/locale/en.json
Length of output: 99
…issues/9272/view-live-consultation-btn
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/ui/button.tsx (2)
67-83: Add JSDoc documentation and improve type safetyThe AuthorizedButton component is well-structured, but could benefit from better documentation and type safety.
Consider adding JSDoc documentation and explicit prop types:
+/** + * A button component that combines authorization logic with standard button functionality. + * @param {Function} authorizeFor - Authorization check function, defaults to always true + * @param {ButtonProps} props - Standard button props + * @returns {JSX.Element} An authorized button component + */ +type AuthorizedButtonProps = AuthorizedElementProps & ButtonProps; + -const AuthorizedButton: React.FC<AuthorizedElementProps & ButtonProps> = ({ +const AuthorizedButton: React.FC<AuthorizedButtonProps> = ({
67-85: Consider documenting the authorization patternThe AuthorizedButton component introduces a reusable pattern for adding authorization to UI elements. Consider documenting this pattern in your component library documentation to encourage consistent usage across the application.
This pattern could be extended to other interactive elements that require authorization checks. Consider creating a HOC (Higher Order Component) or hook if this pattern needs to be reused with other UI components beyond buttons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Patient/PatientHome.tsx(8 hunks)src/components/ui/button.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/components/Patient/PatientHome.tsx
🔇 Additional comments (3)
src/components/ui/button.tsx (3)
7-9: LGTM: Clean import organization
The new imports are properly organized and necessary for the authorization functionality.
85-85: LGTM: Clean export statement
The export statement properly includes the new AuthorizedButton component while maintaining existing exports.
23-23: Verify the impact of primary button style change
The background color change from bg-primary-700 to bg-primary-500 will affect all primary buttons across the application. While this aligns with the PR objectives, we should verify the visual consistency.
✅ Verification successful
The previous command failed because the file type flags weren't supported. Let's try a broader search to find all instances where primary buttons or their styles are used, regardless of file extension.
I want to verify the context of primary button usage and check if there are other button components or variants that might be affected by this change.
Primary button style change is safe to proceed
The change from bg-primary-700 to bg-primary-500 in the button component is part of a consistent styling system:
- The application uses two button implementations:
ButtonV2and the UI button component being modified - The style change is applied to the base UI component which maintains a consistent theme
- The primary variant is widely used across the application for standard actions like submissions and confirmations
- The change maintains the button's visual hierarchy with proper contrast ratios for both light and dark modes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all primary button usage to assess the impact
rg -l "variant=\"primary\"" --type tsx --type jsx
Length of output: 77
Script:
#!/bin/bash
# Search for Button component usage with primary variant
rg -l "variant.*primary"
# Also search for direct class name usage
rg -l "bg-primary-500.*text-white.*shadow.*hover:bg-primary-500/90"
Length of output: 1515
Script:
#!/bin/bash
# Let's check the actual usage context in a few key files
rg "variant.*primary" src/components/Facility/ConsultationCard.tsx -B 2 -A 2
# Check if there are any other button style variants defined
rg "variant.*=.*['\"].*['\"]" src/components/ui/button.tsx -A 2
# Check if there's a ButtonV2 component that might be affected
cat src/components/Common/ButtonV2.tsx
Length of output: 7448
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/ui/button.tsx (2)
67-83: Add component documentationWhile the implementation is solid, please add JSDoc documentation to describe:
- The component's purpose
- The props it accepts
- Usage examples
Example:
+/** + * A button component with built-in authorization checks. + * Combines standard button functionality with authorization logic. + * + * @param {Function} authorizeFor - Authorization check function, defaults to () => true + * @param {ButtonProps} props - Standard button props + * @example + * <AuthorizedButton + * authorizeFor={() => hasPermission('view_consultation')} + * variant="primary" + * > + * View Consultation + * </AuthorizedButton> + */ const AuthorizedButton: React.FC<AuthorizedElementProps & ButtonProps> = ({
67-83: Consider separation of concernsWhile the AuthorizedButton implementation is solid, consider if combining UI and authorization logic in a base UI component is the best architectural approach. Alternative approaches could include:
- Using a higher-order component (HOC) pattern
- Implementing authorization at the container/page level
- Using a provider pattern for authorization
This would keep the button component purely presentational and more reusable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/ui/button.tsx(3 hunks)
🔇 Additional comments (4)
src/components/ui/button.tsx (4)
7-9: LGTM: Imports are properly organized
The new imports for authorization components are correctly structured and necessary for the new functionality.
67-83: LGTM: Well-structured authorized button implementation
The AuthorizedButton component is well-implemented with:
- Proper type definitions
- Good composition of authorization and button functionality
- Correct handling of disabled state
85-85: LGTM: Exports are properly updated
The export statement correctly includes the new AuthorizedButton component alongside existing exports.
23-23: Verify the primary button color change
The background color for primary buttons has been changed from bg-primary-700 to bg-primary-500, making them lighter. While this aligns with the PR objective to match other buttons, please ensure this change:
- Maintains sufficient contrast ratio for accessibility
- Is consistent with the design system
- Won't affect other primary buttons across the application
✅ Verification successful
The previous command failed because the file type wasn't recognized. Let's try with a more flexible approach to:
- Find Button usages with primary variant
- Look for similar button styling patterns in the codebase
Let's try a different approach:
- First identify all relevant files
- Search for button variants and styling patterns
- Look at the complete button component implementation
Primary button color change is consistent with design system
The change from bg-primary-700 to bg-primary-500 is part of a consistent design system implementation. The search results show:
- The
primaryvariant in button.tsx is the source of truth for primary button styling across the application - Other components using primary buttons reference this variant through the
buttonVariantsutility - The color
bg-primary-500is consistently used for primary interactive elements throughout the codebase, as seen in multiple components - The change maintains accessibility with white text contrast and includes proper hover states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of primary button usage to ensure consistency
rg "variant=\"primary\"" --type=tsx
Length of output: 63
Script:
#!/bin/bash
# Search for Button component usage with primary variant using a more flexible pattern
rg -t tsx -t jsx "variant=[\"\']primary" -A 2
# Also search for other instances of similar button styling classes
rg -t tsx -t jsx "bg-primary-[57]00.*text-white.*shadow.*hover:bg-primary-[57]00"
Length of output: 258
Script:
#!/bin/bash
# Find all TypeScript/React files first
fd "\.(tsx|jsx|ts|js)$"
# Then search for Button component usage and styling
rg "variant.*primary|bg-primary-[0-9]00.*text-white" $(fd "\.(tsx|jsx|ts|js)$")
# Look at the button component definition
cat src/components/ui/button.tsx
Length of output: 65403
…ub.com/JavidSumra/care_fe into issues/9272/view-live-consultation-btn
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/ui/button.tsx (2)
67-80: Add TypeScript documentation for the AuthorizedButton componentWhile the implementation is solid, consider adding JSDoc documentation to describe:
- Component purpose
- Props inheritance
- Authorization behavior
Example:
/** * A Button component with built-in authorization checks. * Combines standard button functionality with authorization controls. * * @extends {ButtonProps} * @implements {AuthorizedElementProps} */
67-80: Consider documenting usage pattern for patient-specific buttonsSince this component will be used for the "View Latest Consultation" button, consider:
- Adding usage examples in the component documentation
- Creating a specific authorization preset for patient-related actions
Example documentation:
/** * Usage with patient-specific authorization: * * <AuthorizedButton * authorizeFor={authorizePatientAction('view_consultation')} * variant="primary" * > * View Latest Consultation * </AuthorizedButton> */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/ui/button.tsx(2 hunks)
🔇 Additional comments (3)
src/components/ui/button.tsx (3)
7-9: LGTM: Clean import additions
The new imports are well-organized and necessary for implementing the authorization functionality.
82-82: LGTM: Clean export addition
The AuthorizedButton export is properly added while maintaining existing exports.
68-68: Review the default authorization behavior
The default authorizeFor = () => true might be too permissive. Consider:
- Making authorization required by removing the default
- Or documenting why a permissive default is acceptable
Let's check how this component is used:
✅ Verification successful
Let me check the implementation of NonReadOnlyUsers to understand the authorization pattern better.
Let me check the implementation of NonReadOnlyUsers in the AuthorizeFor utility:
Default authorization behavior is appropriate with proper documentation
The default authorizeFor = () => true is acceptable because:
- The component provides well-documented TypeScript props with examples showing how to restrict access
- The codebase consistently uses
NonReadOnlyUsersauthorization check where needed - The permissive default follows the principle of "secure by usage" rather than "secure by default", allowing developers to explicitly add restrictions where needed, as evidenced by numerous examples in the codebase
- The pattern matches the exported
Anyone = () => trueutility in AuthorizeFor.tsx, showing this is an intentional design choice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find usage patterns of AuthorizedButton to verify authorization handling
rg -l "AuthorizedButton" | xargs rg "authorizeFor"
Length of output: 377
Script:
#!/bin/bash
# Find the implementation of NonReadOnlyUsers
rg "NonReadOnlyUsers"
Length of output: 6715
Script:
#!/bin/bash
# Get the implementation of NonReadOnlyUsers from AuthorizeFor.tsx
cat src/Utils/AuthorizeFor.tsx
Length of output: 1267
package-lock.json
Outdated
There was a problem hiding this comment.
discard changes to the lockfile
|
LGTM |
|
@JavidSumra 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
Bug Fixes
Refactor