Added an option to upload cover image while facility registration (#7297) #9572
Conversation
WalkthroughThis pull request introduces a new feature for uploading facility cover images. A new route Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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/Routers/routes/FacilityRoutes.tsx (1)
18-20: Route addition aligns with feature objectives.This route addition properly passes the
facilityIdparam to theFacilityCovercomponent, facilitating cover image management. Ensure that any external references to this route are documented in the UI flow, so users can navigate intuitively.If you anticipate expansions (like adding sub-routes for images), consider grouping them together under a parent route for better scalability — for instance,
"/facility/:facilityId/cover/*".src/components/Facility/FacilityCover.tsx (2)
19-32: Permission check is correctly enforced but can be centralized.Your
useEffectproperly redirects unauthorized user types away from this page, showing an error message. This pattern is repeated in multiple components (e.g.,FacilityCreate), so consider centralizing permission checks or employing a higher-order component (HOC) to reduce duplication.
56-69: Optional improvement: Provide user feedback on “Update Cover Image” click.Currently, the "Update Cover Image" button navigates back to the facility page. Adding a short success or progress indicator for the cover image upload might improve the user experience.
src/components/Facility/CoverImage.tsx (1)
39-67: File upload logic is robust but could benefit from unified error handling.
async (xhr: XMLHttpRequest) => { ... }uses inline checks to handle status codes. Consider extracting them into a small helper function for reusability.- The
sleep(1000);approach ensures that the facility data is fully updated on the server. Confirm that the UI state doesn’t need a loading indicator if the delay takes longer.src/components/Facility/FacilityCreate.tsx (1)
417-417: Navigation change to cover image improves workflow.Line 417 updates navigation after facility creation to
/facility/${id}/cover, aligning with the new feature. This is logical and consistent with the PR’s objective.If you foresee future steps after uploading the cover, consider a success dialog or flow that confirms successful creation before guiding them to cover image management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Routers/routes/FacilityRoutes.tsx(2 hunks)src/components/Facility/CoverImage.tsx(1 hunks)src/components/Facility/FacilityCover.tsx(1 hunks)src/components/Facility/FacilityCreate.tsx(1 hunks)
🔇 Additional comments (8)
src/Routers/routes/FacilityRoutes.tsx (1)
4-4: Import statement looks good.
The import for FacilityCover is correctly referencing the file path and aligns with the newly added component usage.
src/components/Facility/FacilityCover.tsx (3)
12-14: Interface usage is clear.
The FacilityProps interface clarifies the optional nature of facilityId. The explicit ?: string nicely highlights that the component can render gracefully even if facilityId is absent.
34-73: Overall rendering and user guidance are clear.
- The
crumbsReplacementsmechanism for naming the steps is a nice approach for dynamic breadcrumb labeling. - The guidelines for image size and format help reduce user error.
16-20: Check for potential null states of facilityId.
While destructuring facilityId from props, you’re using it as facilityId in subsequent code. The optional type suggests this might be undefined. You do handle it gracefully with "????" placeholders in crumbs, but confirm no runtime errors occur if it’s indeed undefined.
Add a short inline comment or guard that clarifies fallback behavior when facilityId is undefined.
- const { facilityId } = props;
+ const { facilityId = "" } = props; // Provide a defaultsrc/components/Facility/CoverImage.tsx (4)
18-20: Prop usage is consistent and clearly typed.
The CoverImageProps interface is straightforward, and explicitly requiring a facilityId avoids confusion around null or undefined states.
27-37: Facility data fetch with fallback navigation is well done.
You use useTanStackQueryInstead to fetch facility data, and navigate to "/not-found" on unsuccessful responses. This ensures a consistent error-handling experience.
69-80: Cover image deletion is straightforward but watch for partial updates.
Deleting an image is as simple as calling request(routes.deleteFacilityCoverImage, ...). This is valid, but consider whether the server can return partial successes or advanced status codes to reflect partial failures.
If partial failures are possible, ensure the UI properly notifies or reverts state.
82-104: Avatar components integrated effectively.
AvatarEditModalandAvatarEditableusage is clean and promotes a consistent user interface.- The
classNameusage ensures responsive design.
| import { CoverImage } from "./CoverImage"; | ||
|
|
||
| interface FacilityProps { | ||
| facilityId?: string; |
There was a problem hiding this comment.
why nullable? won't we always have facilityId?
| facilityId?: string; | |
| facilityId: string; |
| import { Cancel, Submit } from "../Common/ButtonV2"; | ||
| import Page from "../Common/Page"; | ||
| import { CoverImage } from "./CoverImage"; | ||
|
|
| title="Cover Image" | ||
| className="h-[90vh]" | ||
| crumbsReplacements={{ | ||
| [facilityId || "????"]: { name: "Upload" }, |
There was a problem hiding this comment.
show the facility name instead of "Upload"
| [facilityId || "????"]: { name: "Upload" }, | |
| [facilityId]: { name: "Upload" }, |
| @@ -14,6 +15,9 @@ import FacilityLocationRoutes from "@/Routers/routes/FacilityLocationRoutes"; | |||
| const FacilityRoutes: AppRoutes = { | |||
| "/facility": () => <FacilityList />, | |||
| "/facility/create": () => <FacilityCreate />, | |||
| "/facility/:facilityId/cover": ({ facilityId }) => ( | |||
There was a problem hiding this comment.
/cover is not very meaningful.
| "/facility/:facilityId/cover": ({ facilityId }) => ( | |
| "/facility/:facilityId/upload-cover-image": ({ facilityId }) => ( |
|
@rithviknishad, I've made the requested changes. Please review them and let me know if further updates are needed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/Facility/FacilityCover.tsx (1)
35-48: Consider combining permission check with data fetchingThe current implementation has separate flows for permission check and data fetching, which could lead to a race condition. Consider combining these checks in the query's onResponse handler.
const { data: facilityData } = useTanStackQueryInstead( routes.getPermittedFacility, { pathParams: { id: facilityId, }, onResponse: ({ res }) => { if (!res?.ok) { navigate("/not-found"); + } else if ( + authUser && + authUser.user_type !== "StateAdmin" && + authUser.user_type !== "DistrictAdmin" && + authUser.user_type !== "DistrictLabAdmin" + ) { + navigate("/facility"); + Notification.Error({ + msg: "You don't have permission to perform this action. Contact the admin", + }); } }, }, ); - -const authUser = useAuthUser(); -useEffect(() => { - if ( - authUser && - authUser.user_type !== "StateAdmin" && - authUser.user_type !== "DistrictAdmin" && - authUser.user_type !== "DistrictLabAdmin" - ) { - navigate("/facility"); - Notification.Error({ - msg: "You don't have permission to perform this action. Contact the admin", - }); - } -}, [authUser]);src/components/Facility/CoverImage.tsx (2)
69-80: Enhance delete operation feedbackThe delete operation could benefit from better error handling and loading state management.
+const [isDeleting, setIsDeleting] = useState(false); const handleCoverImageDelete = async (onError: () => void) => { + setIsDeleting(true); const { res } = await request(routes.deleteFacilityCoverImage, { pathParams: { id: facilityId }, }); if (res?.ok) { Notification.Success({ msg: "Cover image deleted" }); facilityFetch(); setOpenUploadModal(false); } else { + Notification.Error({ msg: res?.data?.detail || "Failed to delete cover image" }); onError(); } + setIsDeleting(false); };
93-100: Enhance user feedback during operationsThe UI could benefit from showing loading states during operations and internationalizing more strings.
<AvatarEditable id="facility-coverimage" imageUrl={facilityData?.read_cover_image_url} - name={facilityData?.name ? facilityData.name : ""} + name={facilityData?.name || t("facility_name_placeholder")} editable={true} + loading={isDeleting} onClick={() => setOpenUploadModal(true)} className="md:mr-2 lg:mr-6 md:w-80 md:h-80 lg:h-100 lg:w-100" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Routers/routes/FacilityRoutes.tsx(2 hunks)src/components/Facility/CoverImage.tsx(1 hunks)src/components/Facility/FacilityCover.tsx(1 hunks)src/components/Facility/FacilityCreate.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Facility/FacilityCreate.tsx
- src/Routers/routes/FacilityRoutes.tsx
🔇 Additional comments (2)
src/components/Facility/FacilityCover.tsx (2)
1-12: Consider using absolute imports consistently
While some imports use absolute paths (e.g., @/components/Common/ButtonV2), others use relative paths (e.g., raviger). Consider using absolute imports consistently throughout the file.
67-71: Consider internationalizing guidelines and validating size limit
The guidelines contain hardcoded strings that should be internationalized. Additionally, verify that the mentioned size limit (1mb) matches the backend validation.
| <Submit | ||
| type="button" | ||
| onClick={() => { | ||
| navigate(`/facility/${facilityId}`); | ||
| }} | ||
| label={"Update Cover Image"} | ||
| /> |
There was a problem hiding this comment.
Fix Submit button behavior
The Submit button currently behaves the same as Cancel, just navigating away. Since it's labeled "Update Cover Image", it should trigger an image update action before navigation.
Consider removing the Submit button since the actual upload is handled by the CoverImage component, or modify it to trigger the upload modal:
<Submit
type="button"
onClick={() => {
- navigate(`/facility/${facilityId}`);
+ setOpenUploadModal(true);
}}
label={"Update Cover Image"}
/>Committable suggestion skipped: line range outside the PR's diff.
| const handleCoverImageUpload = async (file: File, onError: () => void) => { | ||
| const formData = new FormData(); | ||
| formData.append("cover_image", file); | ||
| const url = `${careConfig.apiUrl}/api/v1/facility/${facilityId}/cover_image/`; | ||
|
|
||
| uploadFile( | ||
| url, | ||
| formData, | ||
| "POST", | ||
| { | ||
| Authorization: | ||
| "Bearer " + localStorage.getItem(LocalStorageKeys.accessToken), | ||
| }, | ||
| async (xhr: XMLHttpRequest) => { | ||
| if (xhr.status === 200) { | ||
| await sleep(1000); | ||
| facilityFetch(); | ||
| Notification.Success({ msg: "Cover image updated." }); | ||
| setOpenUploadModal(false); | ||
| } else { | ||
| onError(); | ||
| } | ||
| }, | ||
| null, | ||
| () => { | ||
| onError(); | ||
| }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve upload implementation robustness
Several concerns in the upload implementation:
- The API URL construction should use route configuration instead of string concatenation
- The artificial sleep delay seems unnecessary
- The error handling could be more specific about what went wrong
Consider these improvements:
-const url = `${careConfig.apiUrl}/api/v1/facility/${facilityId}/cover_image/`;
+const url = routes.uploadFacilityCoverImage.path.replace(':id', facilityId);
uploadFile(
url,
formData,
"POST",
{
Authorization:
"Bearer " + localStorage.getItem(LocalStorageKeys.accessToken),
},
async (xhr: XMLHttpRequest) => {
if (xhr.status === 200) {
- await sleep(1000);
facilityFetch();
Notification.Success({ msg: "Cover image updated." });
setOpenUploadModal(false);
} else {
- onError();
+ const errorMessage = xhr.responseText ? JSON.parse(xhr.responseText).detail : "Failed to upload image";
+ Notification.Error({ msg: errorMessage });
+ onError();
}
},
null,
() => {
- onError();
+ Notification.Error({ msg: "Network error while uploading image" });
+ onError();
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleCoverImageUpload = async (file: File, onError: () => void) => { | |
| const formData = new FormData(); | |
| formData.append("cover_image", file); | |
| const url = `${careConfig.apiUrl}/api/v1/facility/${facilityId}/cover_image/`; | |
| uploadFile( | |
| url, | |
| formData, | |
| "POST", | |
| { | |
| Authorization: | |
| "Bearer " + localStorage.getItem(LocalStorageKeys.accessToken), | |
| }, | |
| async (xhr: XMLHttpRequest) => { | |
| if (xhr.status === 200) { | |
| await sleep(1000); | |
| facilityFetch(); | |
| Notification.Success({ msg: "Cover image updated." }); | |
| setOpenUploadModal(false); | |
| } else { | |
| onError(); | |
| } | |
| }, | |
| null, | |
| () => { | |
| onError(); | |
| }, | |
| ); | |
| }; | |
| const handleCoverImageUpload = async (file: File, onError: () => void) => { | |
| const formData = new FormData(); | |
| formData.append("cover_image", file); | |
| const url = routes.uploadFacilityCoverImage.path.replace(':id', facilityId); | |
| uploadFile( | |
| url, | |
| formData, | |
| "POST", | |
| { | |
| Authorization: | |
| "Bearer " + localStorage.getItem(LocalStorageKeys.accessToken), | |
| }, | |
| async (xhr: XMLHttpRequest) => { | |
| if (xhr.status === 200) { | |
| facilityFetch(); | |
| Notification.Success({ msg: "Cover image updated." }); | |
| setOpenUploadModal(false); | |
| } else { | |
| const errorMessage = xhr.responseText ? JSON.parse(xhr.responseText).detail : "Failed to upload image"; | |
| Notification.Error({ msg: errorMessage }); | |
| onError(); | |
| } | |
| }, | |
| null, | |
| () => { | |
| Notification.Error({ msg: "Network error while uploading image" }); | |
| onError(); | |
| }, | |
| ); | |
| }; |
| const { data: facilityData } = useTanStackQueryInstead( | ||
| routes.getPermittedFacility, | ||
| { | ||
| pathParams: { | ||
| id: facilityId, | ||
| }, | ||
| onResponse: ({ res }) => { | ||
| if (!res?.ok) { | ||
| navigate("/not-found"); | ||
| } | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
use useQuery instead
https://docs.ohc.network/docs/care/development/data-fetching-in-care/
There was a problem hiding this comment.
done!!!
@rithviknishad
There was a problem hiding this comment.
review the code yourself :) there are more usages of it
There was a problem hiding this comment.
@rithviknishad
Yeah, I saw that but the code requires reFetch, is there a way to destructure refetch out of useQuery?
There was a problem hiding this comment.
useQuery({ queryKey: [route.path], queryFn: query(route), enabled: shouldFetch })
Is this the right way? like the "enabled" thingy
There was a problem hiding this comment.
you can destructure refetch :) from usequery.
const { refetch } = useQuery(...);
however, the cleaner way to trigger the refetch is by invalidating the query key instead of calling refetch.
There was a problem hiding this comment.
sorry, I couldn't quiet get you
There was a problem hiding this comment.
There was a problem hiding this comment.
@rithviknishad, can you check now
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Facility/FacilityCover.tsx (1)
72-78:⚠️ Potential issueFix Submit button behavior.
The Submit button currently just navigates away, which is the same as Cancel. It should either be removed or modified to trigger the upload action.
-<Submit - type="button" - onClick={() => { - navigate(`/facility/${facilityId}`); - }} - label={"Update Cover Image"} -/>
🧹 Nitpick comments (3)
src/components/Facility/FacilityCover.tsx (3)
22-27: Add error handling for the query.While the query implementation is correct, consider handling potential errors:
- const { data } = useQuery({ + const { data, error } = useQuery({ queryKey: ["facility", facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: facilityId }, }), }); + + useEffect(() => { + if (error) { + Notification.Error({ + msg: "Failed to fetch facility details", + }); + } + }, [error]);
29-42: Refactor permission check for better maintainability.Consider extracting the allowed user types to a constant and using an array includes check:
+const ALLOWED_USER_TYPES = [ + "StateAdmin", + "DistrictAdmin", + "DistrictLabAdmin" +] as const; + export const FacilityCover = (props: FacilityProps) => { // ... useEffect(() => { if ( authUser && - authUser.user_type !== "StateAdmin" && - authUser.user_type !== "DistrictAdmin" && - authUser.user_type !== "DistrictLabAdmin" + !ALLOWED_USER_TYPES.includes(authUser.user_type) ) {
61-65: Improve accessibility of the guidelines list.Add proper ARIA labels and more descriptive text for screen readers:
-<ul className="list-disc pl-6 text-sm text-color-body-dark"> +<ul + className="list-disc pl-6 text-sm text-color-body-dark" + aria-label="Cover image guidelines" +> - <li>Max size for image uploaded should be 1mb</li> - <li>Allowed formats are jpg,png,jpeg</li> - <li>Recommended aspect ratio for the image is 1:1</li> + <li>Maximum file size: 1 megabyte (MB)</li> + <li>Accepted file formats: JPG, PNG, JPEG</li> + <li>Recommended image dimensions: Square (1:1 aspect ratio)</li> </ul>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityCover.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Facility/FacilityCover.tsx (2)
1-14: LGTM! Imports are well-organized.
The imports follow best practices with absolute paths and logical grouping.
15-17: LGTM! Interface follows the suggested non-nullable pattern.
The FacilityProps interface correctly defines facilityId as a required string parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/Facility/CoverImage.tsx (2)
16-16: 🛠️ Refactor suggestionRemove unnecessary sleep import
The
sleeputility import is used for an artificial delay in the upload handler, which is unnecessary and should be removed.
46-74:⚠️ Potential issueAdd file validation and improve error handling
Several security and implementation concerns:
- Missing file type and size validation
- Basic error handling without specific error messages
- URL construction using string concatenation
- Unnecessary sleep delay
Apply these improvements:
const handleCoverImageUpload = async (file: File, onError: () => void) => { + // Validate file type and size + const allowedTypes = ['image/jpeg', 'image/png', 'image/gif']; + const maxSize = 5 * 1024 * 1024; // 5MB + + if (!allowedTypes.includes(file.type)) { + Notification.Error({ msg: "Invalid file type. Please upload a JPEG, PNG, or GIF image." }); + return; + } + + if (file.size > maxSize) { + Notification.Error({ msg: "File size too large. Maximum size is 5MB." }); + return; + } + const formData = new FormData(); formData.append("cover_image", file); - const url = `${careConfig.apiUrl}/api/v1/facility/${facilityId}/cover_image/`; + const url = routes.uploadFacilityCoverImage.path.replace(':id', facilityId); uploadFile( url, formData, "POST", { Authorization: "Bearer " + localStorage.getItem(LocalStorageKeys.accessToken), }, async (xhr: XMLHttpRequest) => { if (xhr.status === 200) { - await sleep(1000); refetchFacility(); Notification.Success({ msg: "Cover image updated." }); setOpenUploadModal(false); } else { - onError(); + const errorMessage = xhr.responseText ? JSON.parse(xhr.responseText).detail : "Failed to upload image"; + Notification.Error({ msg: errorMessage }); + onError(); } }, null, () => { - onError(); + Notification.Error({ msg: "Network error while uploading image" }); + onError(); }, ); };
🧹 Nitpick comments (3)
src/components/Facility/CoverImage.tsx (3)
18-20: Add JSDoc documentation for the interfaceConsider adding documentation to improve code maintainability:
+/** + * Props for the CoverImage component + * @property {string} facilityId - Unique identifier for the facility + */ interface CoverImageProps { facilityId: string; }
76-87: Improve error handling in delete operationThe error handling could be more specific:
const handleCoverImageDelete = async (onError: () => void) => { const { res } = await request(routes.deleteFacilityCoverImage, { pathParams: { id: facilityId }, }); if (res?.ok) { Notification.Success({ msg: "Cover image deleted" }); refetchFacility(); setOpenUploadModal(false); } else { - onError(); + const errorMessage = await res?.text(); + Notification.Error({ + msg: errorMessage ? JSON.parse(errorMessage).detail : "Failed to delete cover image" + }); + onError(); } };
89-111: Add accessibility improvementsThe render implementation looks good but could benefit from accessibility enhancements:
return ( - <div className="my-auto mx-auto flex"> + <div + className="my-auto mx-auto flex" + role="region" + aria-label="Facility cover image" + > <AvatarEditModal title={t("edit_cover_photo")} open={openUploadModal} imageUrl={data?.read_cover_image_url} handleUpload={handleCoverImageUpload} handleDelete={handleCoverImageDelete} onClose={() => setOpenUploadModal(false)} /> <div> <AvatarEditable id="facility-coverimage" imageUrl={data?.read_cover_image_url} name={data?.name ? data.name : ""} editable={true} onClick={() => setOpenUploadModal(true)} + aria-label={t("edit_cover_photo")} className="md:mr-2 lg:mr-6 md:w-80 md:h-80 lg:h-100 lg:w-100" /> </div> </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/CoverImage.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Facility/CoverImage.tsx (1)
35-44: LGTM! Well-implemented refetch function
The refetch implementation properly invalidates the query cache and handles errors appropriately.
| const { data } = useQuery({ | ||
| queryKey: ["facility", facilityId], | ||
| queryFn: query(routes.getPermittedFacility, { | ||
| pathParams: { id: facilityId }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error and loading state handling
The query setup should handle error and loading states to improve user experience:
- const { data } = useQuery({
+ const { data, isError, isLoading } = useQuery({
queryKey: ["facility", facilityId],
queryFn: query(routes.getPermittedFacility, {
pathParams: { id: facilityId },
}),
});
+ if (isLoading) {
+ return <div>Loading...</div>;
+ }
+ if (isError) {
+ return <div>Error loading facility data</div>;
+ }Committable suggestion skipped: line range outside the PR's diff.
|
👋 Hi, @Utkarsh-Anandani, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
no longer relevant in new architecture |
…297)
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
New Features
Bug Fixes