Redesign the Facility Details Page #10380
Conversation
WalkthroughThis pull request refactors the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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
🧹 Nitpick comments (5)
src/components/Facility/FacilityHome.tsx (5)
20-20: Consider removing commented code.The commented import of
ContactLinkshould be removed if it's no longer needed.-// import ContactLink from "@/components/Common/ContactLink";
66-70: Consider moving formatValue to a utility file.The
formatValuefunction could be useful in other components. Consider moving it to a utility file for better reusability.Create a new file
src/Utils/formatting.ts:export const formatValue = (name: string, label: string) => { return name.endsWith(label) ? name.replace(new RegExp(`${label}$`), "").trim() : name; };
207-219: Add aria-label to improve accessibility.The background container should have an aria-label for better screen reader support.
-<div className="group rounded-3xl relative h-64 w-full bg-gradient-to-br from-emerald-400 via-emerald-500 to-emerald-600"> +<div + className="group rounded-3xl relative h-64 w-full bg-gradient-to-br from-emerald-400 via-emerald-500 to-emerald-600" + aria-label={t("facility_header_background")} +>
276-327: Consider extracting card components for better maintainability.The facility details cards could be extracted into separate components to improve code organization and reusability.
Create new components:
// src/components/Facility/FacilityAddressCard.tsx export const FacilityAddressCard = ({ facilityData, className }: { facilityData: FacilityData; className?: string; }) => ( <Card className={className}> <CardContent> {/* Address content */} </CardContent> </Card> ); // src/components/Facility/FacilityOrganizationCard.tsx export const FacilityOrganizationCard = ({ geoOrganization, className }: { geoOrganization: Organization; className?: string; }) => ( <Card className={className}> <CardContent> {/* Organization content */} </CardContent> </Card> );Then update the main component:
-<div className="flex gap-3"> - <Card className="basis-1/2"> - {/* Address content */} - </Card> - <Card className="basis-1/2"> - {/* Organization content */} - </Card> -</div> +<div className="flex gap-3"> + <FacilityAddressCard + facilityData={facilityData} + className="basis-1/2" + /> + <FacilityOrganizationCard + geoOrganization={facilityData.geo_organization} + className="basis-1/2" + /> +</div>
348-348: Consider moving styles to a CSS module.The badge styles could be moved to a CSS module for better maintainability and reuse.
Create
src/components/Facility/FacilityHome.module.css:.featureBadge { @apply flex items-center gap-2 rounded-md bg-blue-100 text-blue-700 hover:bg-blue-200 px-3 py-1; }Then update the component:
-className="flex items-center gap-2 rounded-md bg-blue-100 text-blue-700 hover:bg-blue-200 px-3 py-1" +className={styles.featureBadge}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityHome.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/components/Facility/FacilityHome.tsx (1)
304-304: TODO: Implement location details.The location details section is currently empty. Please implement the location link functionality.
Would you like me to help implement the location details section? I can provide a solution that integrates with a mapping service.
|
Hey Team, It is ready for review and testing |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Facility/FacilityHome.tsx (1)
169-169: Replace hard-coded permission checkThe permission check is currently hard-coded to true, which could lead to security issues.
Consider implementing proper permission validation:
- const hasPermissionToEditCoverImage = true; + const hasPermissionToEditCoverImage = facilityData.permissions?.canEditCoverImage || false;
🧹 Nitpick comments (3)
src/components/Facility/FacilityHome.tsx (3)
66-70: Add input validation to formatValue functionThe function should handle edge cases and special characters in the label.
Consider this safer implementation:
const formatValue = (name: string, label: string) => { + if (!name || !label) return name; + const escapedLabel = label.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); return name.endsWith(label) - ? name.replace(new RegExp(`${label}$`), "").trim() + ? name.replace(new RegExp(`${escapedLabel}$`), "").trim() : name; };
304-304: Implement missing location detailsThe location details section is empty but the UI structure is in place.
Would you like me to help implement the location details section with a map integration or location link?
348-348: Use design system tokens for badge stylingThe badge styling uses direct color values instead of design system tokens.
Consider using design system tokens:
- className="flex items-center gap-2 rounded-md bg-blue-100 text-blue-700 hover:bg-blue-200 px-3 py-1" + className="flex items-center gap-2 rounded-md bg-primary-100 text-primary-700 hover:bg-primary-200 px-3 py-1"
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/Facility/FacilityHome.tsx (3)
65-69: Consider memoizing formatValue function.Since formatValue is used in a map operation, memoizing it could improve performance for large lists.
- const formatValue = (name: string, label: string) => { + const formatValue = useMemo(() => (name: string, label: string) => { return name.endsWith(label) ? name.replace(new RegExp(`${label}$`), "").trim() : name; - }; + }, []);
307-308: Implement location details section.The location details section is empty with a comment indicating pending implementation.
Would you like me to help implement the location details section? I can generate a solution that integrates with mapping services or displays coordinates in a user-friendly format.
298-300: Validate phone number format.The phone number is directly passed to ContactLink without validation. Consider adding format validation or normalization.
+const formatPhoneNumber = (phone: string) => { + // Remove non-numeric characters + const cleaned = phone.replace(/\D/g, ''); + // Format as XXX-XXX-XXXX + return cleaned.replace(/(\d{3})(\d{3})(\d{4})/, '$1-$2-$3'); +}; <ContactLink - tel={String(facilityData?.phone_number)} + tel={formatPhoneNumber(String(facilityData?.phone_number))} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Common/ContactLink.tsx(1 hunks)src/components/Facility/FacilityHome.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Common/ContactLink.tsx (1)
10-11: LGTM! Clean and focused component.The component follows best practices with:
- Type-safe props using discriminated union
- Appropriate icon selection based on contact type
- Standard protocol formatting for href
src/components/Facility/FacilityHome.tsx (2)
53-87: Optimize repeated geo organization rendering.The same geo organization data is rendered multiple times in the component.
This was previously flagged in an earlier review. The suggestion to memoize the rendered organizations still applies:
+ const memoizedGeoOrganizations = useMemo( + () => facilityData.geo_organization && renderGeoOrganizations(facilityData.geo_organization), + [facilityData.geo_organization] + );
43-51: Handle null icon case explicitly.The function can return null when no feature is found or when the icon is undefined. Consider providing a default icon or handling the null case where this function is used.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Address: modify to display address + zipcode Add top padding and maybe adjust the font size for both of the cards (address and geo). |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Facility/FacilityHome.tsx (1)
168-171:⚠️ Potential issueAction Required: Implement proper permission checks in FacilityHome.
The current implementation relies on a hardcoded permission flag with a TODO noting that permissions should come from the backend.
🧹 Nitpick comments (2)
src/components/Facility/FacilityHome.tsx (2)
65-69: Optimize the formatValue function.The current regex implementation recreates the pattern on each call. Consider memoizing the RegExp object or using string methods for better performance.
- return name.endsWith(label) - ? name.replace(new RegExp(`${label}$`), "").trim() - : name; + return name.endsWith(label) + ? name.slice(0, -label.length).trim() + : name;
211-215: Improve image accessibility attributes.The cover image could benefit from better accessibility attributes.
<img src={facilityData.read_cover_image_url} alt={facilityData?.name} + role="img" + aria-label={t("facility_cover_image", { name: facilityData?.name })} className="h-full w-full object-cover rounded-2xl" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityHome.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Facility/FacilityHome.tsx (3)
223-227: Add alt text to Avatar component for accessibility.The Avatar component should have an alt text to improve accessibility.
306-311: Implement the location details section.The location details section is currently empty with a comment placeholder.
Could you clarify if this section should:
- Display coordinates if available in the facility data
- Show a map integration
- Be removed if not needed
277-333: Optimize repeated geo organization rendering.The same geo organization data is rendered in two separate cards, which could impact performance and maintainability.
The geo org labels are already capitalized like |
My bad, meant geo org values 👍 |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Facility/FacilityHome.tsx (1)
138-141: Improve error handling in image upload.The error case only calls the error callback without providing any user feedback. Consider adding a toast notification with the error details.
} else { + toast.error(t("failed_to_upload_image")); onError(); }
🧹 Nitpick comments (2)
src/components/Facility/FacilityHome.tsx (2)
65-69: MoveformatValuefunction outside for reusability.The
formatValuefunction is currently nested insiderenderGeoOrganizations, limiting its reusability. Consider moving it to the module level.+const formatValue = (name: string, label: string) => { + return name.endsWith(label) + ? name.replace(new RegExp(`${label}$`), "").trim() + : name; +}; const renderGeoOrganizations = (geoOrg: Organization) => { const orgParents: OrganizationParent[] = []; - const formatValue = (name: string, label: string) => { - return name.endsWith(label) - ? name.replace(new RegExp(`${label}$`), "").trim() - : name; - };
132-132: Remove arbitrary sleep delay.The
sleep(1000)seems to be an arbitrary delay. If this is meant to handle race conditions or UI feedback, consider using proper loading states instead.- await sleep(1000); + // Use loading state to handle UI feedback + setIsLoading(true); queryClient.invalidateQueries({ queryKey: ["facility", facilityId], }); + setIsLoading(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityHome.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/Facility/FacilityHome.tsx (4)
168-171: Action Required: Implement proper permission checks in FacilityHome.The current implementation relies on a hardcoded permission flag with a TODO noting that permissions should come from the backend.
223-227: Add alt text to Avatar component.
306-311: Implement location details section.The location details section is empty with a comment placeholder. Based on the PR objectives and nihal467's comments, this should display the facility location hyperlink.
Would you like me to help implement this section?
286-293: Remove or implement commented code.The commented-out code for rendering geo organizations suggests uncertainty about the address display implementation. Either remove the commented code or implement the feature.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if geo organizations are used elsewhere in the codebase rg "renderGeoOrganizations" --type ts --type tsxLength of output: 76
🏁 Script executed:
#!/bin/bash # Re-run search for "renderGeoOrganizations" across the repository, # adding a file type for TSX files so that all instances are found. rg "renderGeoOrganizations" --type ts --type-add 'tsx:*.tsx'Length of output: 342
Action: Remove or fully implement the geo organizations rendering
The search confirms that
renderGeoOrganizationsis defined and only referenced in the commented block. This indicates that the feature isn’t active. Based on the review comment, you should either remove the commented code fromsrc/components/Facility/FacilityHome.tsx(and potentially remove the unused function if it isn’t needed elsewhere) or complete its implementation if geo organization details should be displayed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/Facility/FacilityHome.tsx (2)
168-171:⚠️ Potential issueCritical: Replace hardcoded permission with proper permission check.
The hardcoded permission flag (
hasPermissionToEditCoverImage = true) could lead to unauthorized access. This needs to be replaced with a proper permission check.Use the existing permission system:
- const hasPermissionToEditCoverImage = true; + const { checkPermission } = usePermissions(); + const hasPermissionToEditCoverImage = checkPermission("FacilityEditCoverImage");
223-227: 🛠️ Refactor suggestionAdd alt text to Avatar component for accessibility.
The Avatar component should include alt text for better accessibility.
<Avatar name={facilityData.name} + alt={`${facilityData.name} facility avatar`} className="h-20 w-20 rounded-md border-4 border-white shadow-lg" />
🧹 Nitpick comments (2)
src/components/Facility/FacilityHome.tsx (2)
65-69: Consider memoizing the formatValue function.The
formatValuefunction is called multiple times within therenderGeoOrganizationsfunction. Consider memoizing it to prevent unnecessary recalculations.+ const formatValue = useMemo(() => + (name: string, label: string) => { + return name.endsWith(label) + ? name.replace(new RegExp(`${label}$`), "").trim() + : name; + }, + [] + );
286-292: Remove commented code.The commented-out geo organization rendering code should be removed as it's no longer needed, given that the same information is now rendered in a separate card.
- {/* {facilityData.geo_organization && - renderGeoOrganizations( - facilityData.geo_organization, - ).map((org, index) => ( - <span key={index}>{org.value} </span> - ))} */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityHome.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Facility/FacilityHome.tsx (1)
310-311: Add the location link as mentioned in PR comments.The comment
{/* Add Location Link Here */}needs to be implemented. According to the PR comments, this is being added in a different PR, but ensure it's tracked and synchronized with this design.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Facility/FacilityHome.tsx (1)
167-170:⚠️ Potential issueAction Required: Implement proper permission checks in FacilityHome.
The hardcoded permission flag is a security risk. Use the existing permission utilities via the
usePermissionshook.
🧹 Nitpick comments (2)
src/components/Facility/FacilityHome.tsx (2)
66-70: Enhance theformatValuefunction with memoization and edge case handling.The function could be optimized and made more robust:
+ const formatValue = useMemo((name: string, label: string) => { + if (!name || !label) return name; + const sanitizedLabel = label.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); return name.endsWith(label) - ? name.replace(new RegExp(`${label}$`), "").trim() + ? name.replace(new RegExp(`${sanitizedLabel}$`), "").trim() : name; + }, []);
276-276: Enhance responsive design for mobile screens.As per PR feedback, ensure the layout is optimized for various mobile screens:
- <div className="flex flex-col [@media(min-width:60rem)]:flex-row gap-3"> + <div className="grid grid-cols-1 md:grid-cols-2 gap-3">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityHome.tsx(5 hunks)
🔇 Additional comments (3)
src/components/Facility/FacilityHome.tsx (3)
222-226: Add alt text to Avatar component.The Avatar component should have an alt text for accessibility.
285-291: Uncomment and update the address display logic.The commented code for rendering geo organizations should be reviewed and integrated with the address display to ensure comprehensive location information.
Consider this structure:
- {/* {facilityData.geo_organization && - renderGeoOrganizations( - facilityData.geo_organization, - ).map((org, index) => ( - <span key={index}>{org.value} </span> - ))} */} - {facilityData.address} + <div> + {facilityData.address} + {facilityData.geo_organization && ( + <div className="mt-2 text-gray-600"> + {renderGeoOrganizations(facilityData.geo_organization) + .map(org => org.value) + .join(", ")} + </div> + )} + </div>✅ Verification successful
Address display logic update verified.
The suggested changes correctly integrate both the facility's address and its geo organization information. The new structure ensures that the address is always displayed, while geo organization values (if available) are rendered as a comma-separated list in a styled div. This matches the review comment's intent to provide comprehensive location details.
- Address is displayed unconditionally.
- Geo organizations are conditionally rendered with proper styling and displayed as a comma-separated list.
309-309: Add facility location hyperlink as requested.The location link is missing as noted in the PR feedback.
Jacobjeevan
added
needs testing
and removed
question
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Refactor
Style
Bug Fixes