GSoC - Doctor Notes Enhancement #8016
Conversation
|
@UdaySagar-Git is attempting to deploy a commit to the Open Healthcare Network Team on Vercel. A member of the Team first needs to authorize it. |
✅ Deploy Preview for care-egov-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hi, This pr has been automatically marked as stale because it has not had any recent activity. It will be automatically closed if no further activity occurs for 7 more days. Thank you for your contributions. |
|
@rithviknishad can you please remove stale label |
|
👋 Hi, @UdaySagar-Git, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
UdaySagar-Git
force-pushed
the
gsoc-doctor-notes
branch
from
90a82d7
to
618a407
Compare
|
👋 Hi, @UdaySagar-Git, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
👋 Hi, @UdaySagar-Git, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
WalkthroughThe changes consist of multiple updates across various components and files. A new notification event, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant NotesService
User->>UI: Open Patient Notes
UI->>API: Fetch Patient Notes
API->>NotesService: Get Notes for Patient
NotesService-->>API: Return Notes
API-->>UI: Display Notes
User->>UI: Add Note
UI->>API: Send New Note
API->>NotesService: Save Note
NotesService-->>API: Return Note ID
API-->>UI: Update Note List
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/components/Common/MarkdownPreview.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (38)
src/common/hooks/useAuthUser.ts (1)
10-10: Consider adding JSDoc documentation for refetchUser.Since this method is critical for user profile updates and file upload features, adding documentation would help other developers understand its purpose and usage patterns.
+ /** Refreshes the current user data from the backend. + * Used after profile updates, file uploads, or when user data needs synchronization. + * @returns A promise containing the updated user data or error details + */ refetchUser: () => Promise<RequestResult<UserModel>>;src/components/Facility/DoctorNoteReplyPreviewCard.tsx (2)
23-51: Consider accessibility and structural improvements.While the layout works, there are opportunities for enhancement:
- Add ARIA labels for better screen reader support
- Consider extracting the user avatar into a separate component
- Enhance the tooltip implementation with proper ARIA attributes
Consider applying these improvements:
- <div className="mt-3 flex flex-col rounded-lg border border-gray-300 bg-gray-200/50 pt-2 text-gray-800"> + <div + className="mt-3 flex flex-col rounded-lg border border-gray-300 bg-gray-200/50 pt-2 text-gray-800" + role="article" + aria-label="Note reply preview" + > <div className="flex flex-col"> <div className="flex justify-between pl-3"> <div className="flex gap-2"> - <div className="mt-1 flex h-9 w-9 items-center justify-center rounded-full bg-primary text-lg font-semibold text-white"> + <div + className="mt-1 flex h-9 w-9 items-center justify-center rounded-full bg-primary text-lg font-semibold text-white" + role="img" + aria-label={`${parentNote.created_by_object?.first_name || "Unknown"}'s avatar`} + > {parentNote.created_by_object?.first_name[0]} </div>
64-65: Improve content truncation UX.The current implementation truncates content at a fixed height without indicating if content is hidden or providing a way to view more.
Consider adding an expand/collapse mechanism:
- <div className="max-h-14 overflow-hidden pb-2 pl-14 text-sm text-gray-700"> + <div className="pb-2 pl-14 text-sm text-gray-700"> + <div className={`${isExpanded ? '' : 'max-h-14 overflow-hidden'}`}> <MarkdownPreview markdown={parentNote.note} /> + </div> + {!isExpanded && contentHeight > 56 && ( + <button + onClick={() => setIsExpanded(true)} + className="mt-1 text-primary hover:text-primary-dark" + > + Show more + </button> + )} </div>Note: This would require adding state management for
isExpandedand measuring content height.src/components/Facility/PatientConsultationNotesList.tsx (2)
18-19: LGTM! Consider adding JSDoc comments.The new interface properties are well-typed. Consider adding JSDoc comments to document the purpose of these props and their expected usage.
+ /** View mode for the notes list. Defaults to "default-view" */ mode?: "thread-view" | "default-view"; + /** Callback to update the currently viewed thread note */ setThreadViewNote?: (noteId: string) => void;
83-86: Consider memoizing setThreadViewNote to prevent unnecessary re-renders.While the effect logic is correct, if
setThreadViewNoteis recreated frequently in the parent component, it could lead to unnecessary re-renders. Consider memoizing the callback in the parent component usinguseCallback.// In parent component: const setThreadViewNote = useCallback((noteId: string) => { // implementation }, [/* dependencies */]);src/components/Facility/DoctorNote.tsx (2)
13-14: LGTM! Consider adding JSDoc comments.The new props are well-typed and maintain backward compatibility. Consider adding JSDoc comments to document the purpose and usage of these props.
interface DoctorNoteProps { + /** Determines the display mode of notes. "thread-view" shows threaded discussions, "default-view" shows flat list */ mode?: "thread-view" | "default-view"; + /** Callback function to set the currently viewed thread note */ setThreadViewNote?: (noteId: string) => void;
30-30: Consider using dynamic height for better responsiveness.The fixed height of
500pxmight not work well across different screen sizes and devices. Consider using viewport-relative units or dynamic sizing.- className="mt-4 flex h-[500px] grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2" + className="mt-4 flex h-[calc(100vh-200px)] min-h-[500px] grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2"src/components/Common/MentionDropdown.tsx (2)
26-37: Consider scroll position in dropdown positioning.The current position calculation doesn't account for scroll position, which could lead to misalignment. Also, the selected index should be reset when the filter changes.
const [dropdownPosition, setDropdownPosition] = useState({ top: 0, left: 0 }); const [selectedIndex, setSelectedIndex] = useState<number | null>(null); useEffect(() => { if (editorRef.current) { + const scrollTop = window.pageYOffset || document.documentElement.scrollTop; + const scrollLeft = window.pageXOffset || document.documentElement.scrollLeft; setDropdownPosition({ - top: position.top, - left: position.left, + top: position.top + scrollTop, + left: position.left + scrollLeft, }); } }, [position, editorRef]); +useEffect(() => { + setSelectedIndex(null); +}, [filter]);
38-42: Enhance user filtering flexibility.The current filtering only matches username starts-with. Consider implementing a more flexible search that includes partial matches and possibly searching across other user fields (e.g., first_name, last_name).
const filteredUsers = useMemo(() => { - return users.filter((user) => - user.username.toLowerCase().startsWith(filter.toLowerCase()), - ); + const searchTerm = filter.toLowerCase(); + return users.filter((user) => { + const username = user.username.toLowerCase(); + const firstName = user.first_name.toLowerCase(); + const lastName = user.last_name?.toLowerCase() || ''; + return username.includes(searchTerm) || + firstName.includes(searchTerm) || + lastName.includes(searchTerm); + }); }, [users, filter]);src/components/Patient/DiscussionNotesFiles.tsx (4)
29-38: Review and clean up unused interface properties.The
StateInterfacecontains properties likezoom,rotation, and zoom-related flags that don't appear to be used in this component. Consider removing unused properties or documenting their purpose if they're needed for future implementation.
40-51: Enhance code maintainability with constants and utility functions.Consider the following improvements:
- Extract "UNARCHIVED" as a constant since it's used in multiple places.
- Move the offset calculation to a reusable utility function.
+const DEFAULT_TAB = "UNARCHIVED" as const; +const calculateOffset = (page: number, limit: number) => (page - 1) * limit; + export const DiscussionNotesFiles = (props: DiscussionNotesProps) => { const { t } = useTranslation(); const [currentPage, setCurrentPage] = useState(1); const [offset, setOffset] = useState(0); - const [tab, setTab] = useState("UNARCHIVED"); + const [tab, setTab] = useState(DEFAULT_TAB); const handlePagination = (page: number, limit: number) => { - const offset = (page - 1) * limit; + const offset = calculateOffset(page, limit); setCurrentPage(page); setOffset(offset); };
83-86: Use translations for tab labels.The tab labels are hardcoded in English. Use the translation function for internationalization support.
const tabs = [ - { text: "Active Files", value: "UNARCHIVED" }, - { text: "Archived Files", value: "ARCHIVED" }, + { text: t("active_files"), value: "UNARCHIVED" }, + { text: t("archived_files"), value: "ARCHIVED" }, ];
117-121: Simplify editable permission logic.The editable permission check could be simplified and moved to a separate function for better maintainability.
+const canEditFile = (file: FileUploadModel, user: AuthUser) => + file?.uploaded_by?.username === user.username || + ["DistrictAdmin", "StateAdmin"].includes(user.user_type); + <FileBlock file={item} key={item.id} fileManager={fileManager} associating_id={item.associating_id} - editable={ - item?.uploaded_by?.username === authUser.username || - authUser.user_type === "DistrictAdmin" || - authUser.user_type === "StateAdmin" - } + editable={canEditFile(item, authUser)} archivable={true} />src/Utils/useFileUpload.tsx (3)
Line range hint
142-186: Enhance error handling and state management for better UX.Based on reported issues in the PR:
- The error messages could be more user-friendly
- The state should be properly restored after failed uploads
- Consider adding retry logic for failed uploads
Consider this improvement:
const uploadfile = async (data: CreateFileResponse, file: File) => { const url = data.signed_url; const internal_name = data.internal_name; const newFile = new File([file], `${internal_name}`); - return new Promise<void>((resolve, reject) => { + return new Promise<void>(async (resolve, reject) => { + const maxRetries = 3; + let attempts = 0; + + while (attempts < maxRetries) { + try { uploadFile( url, newFile, "PUT", { "Content-Type": file.type }, (xhr: XMLHttpRequest) => { if (xhr.status >= 200 && xhr.status < 300) { setProgress(null); Notification.Success({ - msg: t("file_uploaded"), + msg: t("file_uploaded_success", { fileName: file.name }), }); setError(null); onUpload && onUpload(data); resolve(); } else { + attempts++; Notification.Error({ - msg: t("file_error__dynamic", { statusText: xhr.statusText }), + msg: t("file_error__dynamic", { + fileName: file.name, + statusText: xhr.statusText, + attempt: attempts, + maxRetries + }), }); setProgress(null); - reject(); + if (attempts === maxRetries) reject(); } }, setProgress as any, () => { + attempts++; Notification.Error({ - msg: t("file_error__network"), + msg: t("file_error__network", { + fileName: file.name, + attempt: attempts, + maxRetries + }), }); setProgress(null); - reject(); + if (attempts === maxRetries) reject(); }, ); + break; // Exit loop if upload succeeds + } catch (error) { + if (attempts === maxRetries) throw error; + } + // Add exponential backoff + await new Promise(r => setTimeout(r, Math.pow(2, attempts) * 1000)); + } }); };
Line range hint
108-141: Enhance security measures for medical file uploads.Given this handles medical data:
- The 100MB (
10e7) file size limit might be too permissive- Consider adding virus scanning integration
- Implement strict content-type verification
Consider these security enhancements:
const validateFileUpload = () => { if (files.length === 0) { setError(t("file_error__choose_file")); return false; } for (const file of files) { const filenameLength = file.name.trim().length; if (filenameLength === 0) { setError(t("file_error__file_name")); return false; } - if (file.size > 10e7) { + // Define size limits per file type + const maxSizes = { + 'image': 5 * 1024 * 1024, // 5MB for images + 'audio': 25 * 1024 * 1024, // 25MB for audio + 'document': 10 * 1024 * 1024 // 10MB for documents + }; + const maxSize = maxSizes[type] || 10 * 1024 * 1024; + if (file.size > maxSize) { setError(t("file_error__file_size")); return false; } + // Verify content type matches extension + const declaredType = file.type; + const extension = file.name.split(".").pop()?.toLowerCase(); + const validTypes = { + 'image': /^image\/(jpeg|png|gif|bmp|webp)$/, + 'audio': /^audio\/(mpeg|wav|ogg)$/, + 'document': /^application\/(pdf|msword|vnd.openxmlformats-officedocument.*)$/ + }; + if (!declaredType.match(validTypes[type])) { + setError(t("file_error__invalid_content_type")); + return false; + }
Line range hint
187-229: Improve user experience with better feedback and preview handling.Based on PR feedback:
- Add file preview capabilities
- Improve progress feedback
- Handle network interruptions gracefully
Consider adding a preview component:
type PreviewProps = { file: File; onRemove: () => void; progress: number | null; }; const FilePreview: React.FC<PreviewProps> = ({ file, onRemove, progress }) => { const isImage = ExtImage.includes(file.name.split('.').pop()?.toLowerCase() || ''); return ( <div className="file-preview"> {isImage && ( <img src={URL.createObjectURL(file)} alt={file.name} className="preview-thumbnail" /> )} <div className="preview-info"> <span>{file.name}</span> {progress !== null && ( <progress value={progress} max={100} /> )} </div> <button onClick={onRemove}>Remove</button> </div> ); };Then expose it through the hook:
return { progress, error, validateFiles: validateFileUpload, handleCameraCapture: () => setCameraModalOpen(true), handleAudioCapture: () => setAudioModalOpen(true), handleFileUpload: handleUpload, Dialogues, Input, fileNames: uploadFileNames, files: files, + FilePreview, // Add the preview component setFileNames: setUploadFileNames, setFileName: (name: string, index = 0) => {src/components/Common/FilePreviewDialog.tsx (2)
Line range hint
153-160: Enhance iframe security configuration.The current iframe implementation has security concerns:
- Empty sandbox attribute is too restrictive and might break legitimate previews.
- Direct URL usage without validation could pose security risks.
Consider these security enhancements:
<iframe - sandbox="" + sandbox="allow-scripts allow-same-origin" title="Source Files" - src={fileUrl} + src={validateFileUrl(fileUrl)} className="h-[75vh] w-full" />Add a URL validation helper:
const validateFileUrl = (url: string): string => { try { const validatedUrl = new URL(url); // Ensure URL is from trusted domains if (!validatedUrl.origin.endsWith('your-domain.com')) { throw new Error('Invalid file URL'); } return url; } catch { console.error('Invalid file URL'); return ''; } };
Line range hint
161-167: Enhance accessibility and user experience.Consider the following improvements for better accessibility and user experience:
- Add more descriptive error messages for unsupported file types.
- Improve screen reader support with ARIA labels.
- Add keyboard shortcuts for common actions.
Here's a suggested implementation:
<div className="flex h-full w-full flex-col items-center justify-center"> <CareIcon icon="l-file" - className="mb-4 text-5xl text-secondary-600" + className="mb-4 text-5xl text-secondary-600" + aria-hidden="true" /> - {t("file_preview_not_supported")} + <p role="alert"> + {t("file_preview_not_supported_description", { + fileType: file_state.extension.toUpperCase(), + supportedTypes: previewExtensions.join(', ').toUpperCase() + })} + </p> </div>Also consider adding keyboard shortcuts:
useEffect(() => { const handleKeyPress = (e: KeyboardEvent) => { if (e.key === '+' && e.ctrlKey) handleZoomIn(); if (e.key === '-' && e.ctrlKey) handleZoomOut(); if (e.key === 'r' && e.ctrlKey) handleRotate(90); }; window.addEventListener('keydown', handleKeyPress); return () => window.removeEventListener('keydown', handleKeyPress); }, []);src/components/Facility/PatientNotesSlideover.tsx (2)
12-16: Consider removing .js extensions from importsTypeScript/webpack typically handles module resolution without requiring explicit .js extensions. These extensions might cause issues with some bundlers and are generally unnecessary in TypeScript projects.
-import { PATIENT_NOTES_THREADS } from "@/common/constants.js"; -import DoctorNoteReplyPreviewCard from "./DoctorNoteReplyPreviewCard.js"; -import useNotificationSubscriptionState from "@/common/hooks/useNotificationSubscriptionState.js"; -import AuthorizedChild from "../../CAREUI/misc/AuthorizedChild.js"; +import { PATIENT_NOTES_THREADS } from "@/common/constants"; +import DoctorNoteReplyPreviewCard from "./DoctorNoteReplyPreviewCard"; +import useNotificationSubscriptionState from "@/common/hooks/useNotificationSubscriptionState"; +import AuthorizedChild from "../../CAREUI/misc/AuthorizedChild";
Line range hint
79-95: Consider enhancing error handlingThe function implementation looks good overall, but consider adding explicit error handling for failed requests.
const onAddNote = async () => { if (!/\S+/.test(noteField)) { Notification.Error({ msg: "Note Should Contain At Least 1 Character", }); return; } const { res, data } = await request(routes.addPatientNote, { pathParams: { patientId: patientId }, body: { note: noteField, consultation: consultationId, thread, reply_to: reply_to?.id, }, }); + if (!res?.ok) { + Notification.Error({ + msg: "Failed to add note. Please try again.", + }); + return; + } if (res?.status === 201) { Notification.Success({ msg: "Note added successfully" }); setNoteField(""); setState({ ...state, cPage: 1 }); setReload(true); setReplyTo(undefined); } return data?.id; };src/components/Notifications/NotificationsList.tsx (1)
76-78: Consider adding data validation for required properties.The URL construction looks good, but consider adding validation for required properties (facility, patient, consultation) to prevent undefined values in the URL.
case "PATIENT_NOTE_ADDED": + if (!data.facility || !data.patient || !data.consultation) { + console.warn('Missing required properties for PATIENT_NOTE_ADDED URL'); + return '#'; + } return `/facility/${data.facility}/patient/${data.patient}/consultation/${data.consultation}/notes`; case "PATIENT_NOTE_MENTIONED": + if (!data.facility || !data.patient || !data.consultation) { + console.warn('Missing required properties for PATIENT_NOTE_MENTIONED URL'); + return '#'; + } return `/facility/${data.facility}/patient/${data.patient}/consultation/${data.consultation}/notes`;src/components/Users/UserProfile.tsx (5)
Line range hint
13-14: Improve date validation logic and error message.The current date validation has two issues:
- The age requirement (17 years) should be configurable rather than hardcoded
- The error message doesn't clearly communicate the age requirement to users
Consider this improvement:
-if (!dayjs(states.form[field]).isValid() || - dayjs(states.form[field]).isAfter(dayjs().subtract(17, "year"))) { - errors[field] = "Enter a valid date of birth"; +const MIN_AGE_REQUIREMENT = 17; // Move to constants +if (!dayjs(states.form[field]).isValid() || + dayjs(states.form[field]).isAfter(dayjs().subtract(MIN_AGE_REQUIREMENT, "year"))) { + errors[field] = `You must be at least ${MIN_AGE_REQUIREMENT} years old`;
Line range hint
201-211: Extract password validation logic into a reusable function.The password validation logic is duplicated between the UI validation rules and the
validateNewPasswordfunction. This could lead to inconsistencies if validation rules change.Consider extracting the validation into a shared utility:
// passwordUtils.ts export const PASSWORD_RULES = { minLength: 8, requireNumbers: true, requireMixedCase: true }; export function validatePassword(password: string): { isValid: boolean; errors: string[] } { const errors: string[] = []; if (password.length < PASSWORD_RULES.minLength) { errors.push(`Password must be at least ${PASSWORD_RULES.minLength} characters`); } if (!/\d/.test(password)) { errors.push("Password must contain at least one number"); } if (password === password.toLowerCase() || password === password.toUpperCase()) { errors.push("Password must contain both uppercase and lowercase letters"); } return { isValid: errors.length === 0, errors }; }
Line range hint
321-368: Add loading state and prevent concurrent submissions.The form submission handler doesn't prevent multiple concurrent submissions or provide visual feedback during submission.
Consider these improvements:
+const [isSubmitting, setIsSubmitting] = useState(false); const handleSubmit = async (e: FormEvent) => { e.preventDefault(); + if (isSubmitting) return; + const validForm = validateForm(); if (validForm) { + setIsSubmitting(true); try { const data = { // ... existing data preparation }; const { res } = await request(routes.partialUpdateUser, { pathParams: { username: authUser.username }, body: data, }); if (res?.ok) { Notification.Success({ msg: "Details updated successfully", }); await refetchUserData(); setShowEdit(false); } + } catch (error) { + Notification.Error({ + msg: "Failed to update profile. Please try again.", + }); + } finally { + setIsSubmitting(false); + } } };Then update the submit button:
-<Submit onClick={handleSubmit} label={t("update")} disabled={!dirty} /> +<Submit + onClick={handleSubmit} + label={isSubmitting ? t("updating...") : t("update")} + disabled={!dirty || isSubmitting} +/>
Line range hint
481-507: Modernize file upload implementation.The current implementation uses XMLHttpRequest directly and could benefit from modern approaches and better error handling.
Consider using the Fetch API with proper error handling:
const handleAvatarUpload = async (file: File, onError: () => void) => { const formData = new FormData(); formData.append("profile_picture", file); try { const response = await fetch( `${careConfig.apiUrl}/api/v1/users/${authUser.username}/profile_picture/`, { method: 'POST', headers: { 'Authorization': `Bearer ${localStorage.getItem(LocalStorageKeys.accessToken)}` }, body: formData } ); if (!response.ok) { throw new Error(`Upload failed: ${response.statusText}`); } await sleep(1000); await refetchUser(); Notification.Success({ msg: "Profile picture updated." }); setEditAvatar(false); } catch (error) { console.error('Upload failed:', error); onError(); Notification.Error({ msg: "Failed to upload profile picture. Please try again." }); } };
Line range hint
590-650: Enhance form accessibility.The form lacks proper ARIA attributes and keyboard navigation support.
Consider these accessibility improvements:
<form + role="form" + aria-label="User Profile Form" action="#" method="POST" > <div className="shadow sm:rounded-md"> <div className="bg-white px-4 pt-5"> <div + role="group" + aria-labelledby="personal-info-heading" className="grid grid-cols-6 gap-4" > + <h2 + id="personal-info-heading" + className="sr-only" + > + Personal Information + </h2> {/* Existing form fields */} </div> </div> <div className="bg-secondary-50 px-4 py-3 text-right sm:px-6"> <Submit onClick={handleSubmit} label={t("update")} disabled={!dirty} + aria-label="Update profile information" /> </div> </div> </form>src/Redux/api.tsx (1)
1167-1173: Consider adding a create endpoint for consultation file uploads.While the list endpoint is well-implemented, consider adding a corresponding create endpoint to handle file uploads specifically for consultations. This would provide a more complete API surface for the consultation files feature.
Example addition:
// Consultation FileUploads listConsultationFileUploads: { path: "/api/v1/consultation/{consultation_external_id}/files/", method: "GET", TRes: Type<PaginatedResponse<FileUploadModel>>(), }, + createConsultationFileUpload: { + path: "/api/v1/consultation/{consultation_external_id}/files/", + method: "POST", + TBody: Type<CreateFileRequest>(), + TRes: Type<CreateFileResponse>(), + },src/components/Common/MarkdownPreview.tsx (2)
6-9: ExtendCustomLinkPropsto Include Standard Anchor AttributesCurrently,
CustomLinkPropsonly includesclassNameanddata-username. To ensure that all standard anchor tag properties are available in theCustomLinkcomponent, consider extendingCustomLinkPropsfromReact.ComponentProps<"a">.Apply this diff to update the interface:
-interface CustomLinkProps { - className?: string; - "data-username"?: string; -} +interface CustomLinkProps extends React.ComponentProps<"a"> { + "data-username"?: string; +}
38-45: Improve Username Matching in RegexThe current regex
/@(\w+)/gmay not match all valid usernames if they include characters like dots, dashes, or underscores. If usernames can contain such characters, consider updating the regex to accommodate them.For example:
-.replace(/@(\w+)/g, (_, username) => { +.replace(/@([a-zA-Z0-9._-]+)/g, (_, username) => {src/components/Facility/PatientNotesDetailedView.tsx (1)
112-119: Enhance accessibility by adding aria-labels to buttons.To improve accessibility, consider adding
aria-labelattributes to the close button for screen readers.Apply this diff:
<button onClick={() => setThreadViewNote?.("")} className="btn btn-default" + aria-label="Close detailed view" > <CareIcon icon="l-times" className="mr-1 text-lg" /> Close </button>src/components/Facility/ConsultationDoctorNotes/index.tsx (3)
115-132: Consider extracting theTabscomponent into a separate file.The
Tabscomponent is a reusable UI component that could be extracted into a separate file for better modularity and maintainability. This would also make it easier to reuse the component in other parts of the application.
Line range hint
133-193: Improve the layout and responsiveness of the component.The changes introduce a new layout structure using a combination of flexbox and conditional rendering based on the
threadViewNotestate. While this enhances the user interface, there are a few areas that could be improved:
- Consider using CSS grid instead of flexbox for a more responsive layout that adapts to different screen sizes.
- Ensure that the component is fully responsive and provides a good user experience on smaller screens, such as mobile devices.
- Use semantic HTML elements, such as
<section>or<article>, to improve the structure and accessibility of the component.
Based on the search results, I can now provide a final response regarding the error handling in the
onAddNotefunction.Error handling needs improvement in the
onAddNotefunction.The current implementation has basic validation for empty notes but lacks proper error handling for the API request. The function should:
- Handle network errors and failed requests
- Show appropriate error notifications to users
- Maintain consistent state in case of failures
Here's what's missing:
- No try-catch block around the
requestcall- No error handling for non-201 status codes
- No state cleanup in case of failures
🔗 Analysis chain
Line range hint
66-86: Verify error handling for theonAddNotefunction.The
onAddNotefunction now returns theidof the added note, which is a useful enhancement for further processing. However, it's important to ensure proper error handling in case the request fails or returns an unexpected status code.To verify the error handling, run the following script:
Let me try a different search pattern to locate the
onAddNotefunction implementation.
Let me check how the
requestfunction handles errors to complete the verification.
Let me try a different search pattern to find error handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in the `onAddNote` function. # Test: Search for the `onAddNote` function. # Expect: The function should have a catch block or error handling logic. ast-grep --pattern $'onAddNote = async () => { $$$ }'Length of output: 484
Script:
#!/bin/bash # Search for the onAddNote function implementation ast-grep --pattern $'const onAddNote = async $_$ => { $$$ }' # Also search with ripgrep to ensure we don't miss any implementation rg "onAddNote\s*=\s*async" -A 20Length of output: 6492
Script:
#!/bin/bash # Search for the request function implementation and error handling rg "request\s*=.*=>" -A 10 # Also search for any try-catch blocks around request calls rg "try\s*{.*request" -A 10Length of output: 164
Script:
#!/bin/bash # Search for try-catch blocks with request rg "try.*\{" -A 15 --multiline # Search for the request function definition rg "export.*request" -A 10Length of output: 85309
src/components/Facility/PatientNoteCard.tsx (2)
205-205: Simplify function call with optional chainingYou can simplify the function call by using optional chaining.
Apply this diff to simplify the function call:
-setReplyTo && setReplyTo(note); +setReplyTo?.(note);🧰 Tools
🪛 Biome
[error] 205-205: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
173-173: Safely access the first character of the first nameTo prevent potential errors if
first_nameis undefined or an empty string, consider safely accessing the first character using optional chaining and a fallback value.Apply this diff to enhance safety:
-{note.created_by_object?.first_name[0]} +{note.created_by_object?.first_name?.[0] || ''}src/components/Common/RichTextEditor.tsx (3)
29-35: Consider renaming the 'onChange' prop for clarityThe prop
onChangeis being aliased tosetMarkdown, which may cause confusion sinceonChangeis commonly associated with event handlers. To improve clarity, consider either:
- Renaming the prop to
setMarkdownin the parent component, reflecting its purpose as a state setter.- Keeping the original prop name
onChangeand using it consistently throughout the component.This will enhance readability and maintain consistency in naming conventions.
402-467: Enhance accessibility by adding 'aria-label' attributes to toolbar buttonsTo improve accessibility for users utilizing assistive technologies:
- Add descriptive
aria-labelattributes to each toolbar button to convey their function.- Ensure the labels are clear and concise (e.g.,
aria-label="Bold"for the bold button).This will make the editor more inclusive and user-friendly.
205-293: Refactor list handling functions to reduce code duplicationThe functions
handleOrderedListandhandleUnorderedListcontain similar logic for manipulating list formatting. To improve maintainability:
- Abstract the common functionality into a helper function that accepts parameters to distinguish between ordered and unordered lists.
- Reduce repeated code, which simplifies future updates and decreases the potential for bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
src/Locale/en.json(1 hunks)src/Redux/api.tsx(3 hunks)src/Utils/useFileUpload.tsx(1 hunks)src/common/constants.tsx(1 hunks)src/common/hooks/useAuthUser.ts(1 hunks)src/components/Common/FilePreviewDialog.tsx(1 hunks)src/components/Common/MarkdownPreview.tsx(1 hunks)src/components/Common/MentionDropdown.tsx(1 hunks)src/components/Common/RichTextEditor.tsx(1 hunks)src/components/Facility/ConsultationDetails/index.tsx(2 hunks)src/components/Facility/ConsultationDoctorNotes/index.tsx(7 hunks)src/components/Facility/DoctorNote.tsx(2 hunks)src/components/Facility/DoctorNoteReplyPreviewCard.tsx(2 hunks)src/components/Facility/PatientConsultationNotesList.tsx(4 hunks)src/components/Facility/PatientNoteCard.tsx(5 hunks)src/components/Facility/PatientNotesDetailedView.tsx(1 hunks)src/components/Facility/PatientNotesSlideover.tsx(6 hunks)src/components/Facility/models.tsx(3 hunks)src/components/Notifications/NotificationsList.tsx(1 hunks)src/components/Patient/DiscussionNotesFiles.tsx(1 hunks)src/components/Patient/PatientNotes.tsx(5 hunks)src/components/Users/UserProfile.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Locale/en.json
🧰 Additional context used
🪛 Biome
src/components/Facility/PatientNoteCard.tsx
[error] 205-205: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (36)
src/common/hooks/useAuthUser.ts (1)
10-10: LGTM! The refetchUser signature is well-typed.
The Promise<RequestResult> return type properly captures the asynchronous nature of user data refresh and aligns well with TypeScript best practices.
src/components/Facility/DoctorNoteReplyPreviewCard.tsx (2)
Line range hint 1-17: LGTM! Well-structured imports and interface definition.
The component's interface is clearly defined with proper TypeScript types, and the imports are well-organized.
19-21: LGTM! Good use of early return pattern.
The early return pattern improves code readability and provides a clean fallback to children when parentNote is undefined.
src/components/Facility/PatientConsultationNotesList.tsx (2)
33-34: LGTM!
The props destructuring is correct, with appropriate default values.
116-117: Verify DoctorNote component interface.
Please ensure that the DoctorNote component's interface has been updated to accept these new props.
✅ Verification successful
Props are properly typed and used in DoctorNote component
The verification shows that:
DoctorNotePropsinterface correctly includes both props:mode?: "thread-view" | "default-view"setThreadViewNote?: (noteId: string) => void
- The props are properly used in the component's logic for conditional rendering and event handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if DoctorNote component properly types these props
# Test: Search for DoctorNote interface/props type definition
ast-grep --pattern 'interface DoctorNoteProps {
$$$
}'
# Test: Search for mode and setThreadViewNote usage in DoctorNote
rg -A 5 'mode.*thread-view|setThreadViewNote' src/components/Facility/DoctorNote.tsx
Length of output: 1551
src/components/Facility/DoctorNote.tsx (3)
18-26: LGTM!
Clean and consistent props destructuring.
42-42: LGTM!
Appropriate styling for a chat-like interface with inverse scrolling.
Line range hint 1-91: Verify integration with all PR features.
While the component handles the reply feature well, please ensure it properly integrates with other features mentioned in the PR objectives:
- User profile images
- Markdown support
- File upload options
- User tagging mechanism
✅ Verification successful
All PR features are properly integrated through PatientNoteCard component
Based on the verification results, the DoctorNote component correctly integrates with all required features through its child component PatientNoteCard:
- Markdown support: Implemented via
<MarkdownPreview>component - File uploads: Handled through
filesprop and FilePreviewDialog - User tagging: Supported via
mentioned_usersprop in MarkdownPreview - Profile images: Basic avatar support shown through user initials in a circular div
- Reply feature: Implemented through
allowReplyandsetReplyToprops
The component architecture properly delegates these features to PatientNoteCard, which contains the necessary implementations for all required functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration with other features
# Test 1: Check if PatientNoteCard component handles markdown and file previews
echo "Checking PatientNoteCard implementation..."
ast-grep --pattern 'interface PatientNoteCardProps {
$$$
}'
# Test 2: Check for user tagging related props
echo "Checking user tagging integration..."
rg -l "userMentions|taggedUsers|onMention" --type typescript
# Test 3: Check for file upload related components
echo "Checking file upload integration..."
rg -l "FileUpload|uploadFile|handleFileUpload" --type typescript
Length of output: 522
Script:
#!/bin/bash
# Let's try a different approach with correct file extensions and more specific patterns
# Check PatientNoteCard implementation
echo "Checking PatientNoteCard.tsx for feature support..."
rg -A 10 "interface PatientNoteCardProps" --type ts --type tsx
# Check for markdown-related imports and components
echo "Checking markdown support..."
rg -l "markdown|Markdown|ReactMarkdown" --type ts --type tsx
# Check for file upload and attachments
echo "Checking file attachments..."
rg -l "attachment|Attachment|upload" --type ts --type tsx
# Check for user mentions/tagging
echo "Checking user mentions..."
rg -l "@mention|mention|tagged" --type ts --type tsx
# Check for avatar/profile image
echo "Checking profile images..."
rg -l "avatar|Avatar|profile.*image" --type ts --type tsx
Length of output: 813
Script:
#!/bin/bash
# Let's try without specifying file types since ripgrep seems to have issues with tsx/ts types
# Check PatientNoteCard implementation
echo "Checking PatientNoteCard for feature support..."
rg -A 10 "interface PatientNoteCardProps"
# Check for markdown-related imports and components
echo "Checking markdown support..."
rg -l "markdown|Markdown|ReactMarkdown"
# Check for file upload and attachments
echo "Checking file attachments..."
rg -l "attachment|Attachment|upload"
# Check for user mentions/tagging
echo "Checking user mentions..."
rg -l "@mention|mention|tagged"
# Check for avatar/profile image
echo "Checking profile images..."
rg -l "avatar|Avatar|profile.*image"
# Also check the specific PatientNoteCard file content
echo "Checking PatientNoteCard content..."
fd -g "*PatientNoteCard.tsx" -x cat {}
Length of output: 16716
src/components/Common/MentionDropdown.tsx (2)
1-11: LGTM! Well-structured interface and imports.
The props interface is well-defined with clear types, and all imports are necessary for the component's functionality.
1-113: Verify integration with markdown editor.
Let's ensure this component is properly integrated with the markdown editor component.
✅ Verification successful
Integration with markdown editor is properly implemented
The MentionsDropdown component is correctly integrated with RichTextEditor:
- All required props are properly passed:
onSelect,position,editorRef, andfilter - Mentions functionality is properly handled with cursor positioning and text insertion
- Component is rendered conditionally based on
showMentionsstate - Keyboard navigation and event handling are implemented correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration between MentionsDropdown and markdown editor
# Find markdown editor files that might use MentionsDropdown
echo "Searching for markdown editor integration..."
rg -l "MentionsDropdown" --type ts --type tsx
# Check for proper props passing
echo -e "\nChecking proper props usage..."
ast-grep --pattern 'import { $_ } from "$_/MentionsDropdown"
$$$
<MentionsDropdown
$$$
/>'
Length of output: 344
Script:
#!/bin/bash
# Let's try a different approach to find the integration
# Search for files containing MentionsDropdown with proper file extensions
echo "Searching for MentionsDropdown usage..."
rg -l "MentionsDropdown" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts
# Look for markdown editor related files
echo -e "\nSearching for markdown editor files..."
rg -l "markdown|editor" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts
# Get content of files that might show the integration
echo -e "\nChecking content of potential integration files..."
rg -A 5 -B 5 "MentionsDropdown" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts
Length of output: 3903
Script:
#!/bin/bash
# Let's examine the RichTextEditor integration more closely
# Get the full content of RichTextEditor to verify proper integration
echo "Checking RichTextEditor implementation..."
cat src/components/Common/RichTextEditor.tsx
# Look for any related test files
echo -e "\nSearching for related test files..."
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts RichTextEditor
Length of output: 23959
src/components/Patient/DiscussionNotesFiles.tsx (1)
20-27: Consider making critical ModalDetails properties required.
The ModalDetails interface has all optional properties. If certain properties are always needed when using this interface (e.g., id, name), consider making them required by removing the ? optional modifier.
src/components/Patient/PatientNotes.tsx (4)
16-16: LGTM! Type safety and i18n improvements.
Good improvements:
- Proper typing for props instead of using
any - Integration of i18n support for better internationalization
Also applies to: 20-21
26-26: LGTM! Proper translation hook setup.
The translation hook is correctly initialized and integrated with the component.
Line range hint 1-166: Verify integration with other PR features.
This component should integrate with several features mentioned in the PR objectives:
- File upload options
- User tagging mechanism
- Profile image support
#!/bin/bash
# Check for file upload integration
echo "Checking for file upload components..."
ast-grep --pattern 'import $_ from $file' | rg 'upload|file'
# Check for user tagging integration
echo "Checking for user tagging components..."
ast-grep --pattern 'import $_ from $file' | rg 'tag|mention|user'
# Check for profile image integration
echo "Checking for profile image components..."
ast-grep --pattern 'import $_ from $file' | rg 'profile|avatar|image'112-113: Verify mobile responsiveness of the notes container.
The styling changes look good, but given the PR feedback about mobile view issues:
✅ Verification successful
Mobile responsiveness implementation looks appropriate
The component implements responsive design patterns correctly:
- Uses Tailwind's responsive prefixes (
sm:) to adjust margins and padding for different screen sizes - Has proper flex layout with
h-screenfor full viewport height - Container adapts with smaller margins and padding on mobile (
mx-3 my-2 p-2) and larger ones on desktop (sm:mx-10 sm:my-5 sm:p-5)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for responsive design patterns in the codebase
echo "Checking for responsive design patterns..."
rg -l 'sm:|md:|lg:|xl:' src/components/Patient/
# Search for mobile-specific styles or components
echo "Checking for mobile-specific implementations..."
rg -l 'mobile|responsive|screen' src/components/Patient/
Length of output: 469
Script:
#!/bin/bash
# Let's try a more specific search for responsive patterns in PatientNotes.tsx
echo "Checking PatientNotes.tsx for responsive patterns..."
rg 'sm:|md:|lg:|xl:' src/components/Patient/PatientNotes.tsx -A 2
# Check for mobile-specific styles in the component
echo "Checking mobile-specific styles in PatientNotes.tsx..."
rg 'mobile|screen' src/components/Patient/PatientNotes.tsx
# Look for viewport-related utilities
echo "Checking for viewport utilities..."
rg 'useWindowSize|useMediaQuery|window\.innerWidth' src/components/Patient/PatientNotes.tsx
Length of output: 860
src/Utils/useFileUpload.tsx (1)
Line range hint 64-74: Consider enhancing file type security measures.
While exporting the image extensions is useful for reusability, consider adding additional security measures for medical-related file uploads:
- Add MIME type validation in addition to extension checks
- Consider using a content-type detection library
- Document the security implications of allowing certain formats like SVG
Let's verify if MIME type validation is consistently applied:
src/components/Facility/PatientNotesSlideover.tsx (3)
29-29: LGTM! Good internationalization practice
The addition of useTranslation hook and its usage throughout the component ensures proper internationalization support.
180-182: LGTM! Appropriate width adjustment
The width increase to 430px provides more space for the enhanced features while maintaining a reasonable size for the slideover panel.
224-241: Verify accessibility of the rich text editor
The RichTextEditor integration looks good and properly handles authorization. However, we should verify its accessibility features.
✅ Verification successful
Rich text editor has adequate keyboard accessibility implementation
The RichTextEditor component implements sufficient keyboard accessibility features:
- Proper keyboard navigation with Enter key handling for lists and quotes
- Escape key support for closing mentions dropdown
- Full keyboard support for text formatting and link insertion
- ARIA-compatible toolbar buttons with tooltips
- Custom text area with maintained focus management
The implementation follows accessibility best practices by:
- Preserving focus states after operations
- Providing clear visual feedback through tooltips
- Maintaining proper keyboard navigation flow
- Including descriptive labels and IDs for form controls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for aria attributes and role definitions in the RichTextEditor component
rg -A 5 'aria-|role=' src/components/Common/RichTextEditor.tsx
# Search for keyboard event handlers that might affect accessibility
ast-grep --pattern 'onKeyDown|onKeyPress|onKeyUp' src/components/Common/RichTextEditor.tsx
Length of output: 157
Script:
#!/bin/bash
# First, let's verify if the RichTextEditor file exists and check its contents
fd RichTextEditor.tsx
# If it exists in a different location, let's search the entire codebase for rich text editor accessibility patterns
rg -i "aria-|role=|keyboard|a11y" -g "**/*RichTextEditor*"
# Let's also check if we're using any known rich text editor libraries that might handle accessibility
rg -l "@tiptap|slate-react|draft-js|react-quill" package.json
# Search for any accessibility-related tests
rg -l "accessibility|a11y" -g "*test*" -g "*spec*"
Length of output: 444
Script:
#!/bin/bash
# Let's examine the keyboard event handlers in RichTextEditor.tsx
rg -A 10 "handleEscape|onKeyDown" src/components/Common/RichTextEditor.tsx
# Check if we're using any markdown editor component that might handle accessibility
cat src/components/Common/RichTextEditor.tsx
# Look for any editor-related components that might be imported
rg -l "import.*Editor" src/components/Common/RichTextEditor.tsx
Length of output: 25341
src/components/Facility/ConsultationDetails/index.tsx (3)
39-39: LGTM!
The import statement follows the project's import organization pattern and uses appropriate relative path notation.
60-60: LGTM! Clean integration with existing tab system.
The new tab is well-integrated into the existing system, leveraging the flexible tab architecture without requiring changes to the core navigation logic.
60-60: Verify translation key for the new tab.
Ensure that the translation key CONSULTATION_TAB__DISCUSSION_NOTES_FILES is defined in the localization files.
✅ Verification successful
Translation key is properly defined
The translation key CONSULTATION_TAB__DISCUSSION_NOTES_FILES is correctly defined in the localization files, supporting the new tab implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if translation key exists in localization files
# Search for the translation key in localization files
rg "CONSULTATION_TAB__DISCUSSION_NOTES_FILES" -g "*.json"
Length of output: 408
Script:
#!/bin/bash
# Check for translation keys in all locale files
fd -e json -d 2 . src/Locale --exec cat {} \; | jq -r 'select(.CONSULTATION_TAB__DISCUSSION_NOTES_FILES != null) | keys[] | select(. == "CONSULTATION_TAB__DISCUSSION_NOTES_FILES")'
Length of output: 226
src/components/Notifications/NotificationsList.tsx (2)
76-78: LGTM: Seamless integration with existing notification system.
The new notification types are well-integrated with the existing notification system. They will automatically work with all features including:
- Read/unread status management
- Notification filtering
- Push notifications
77-78: Verify if PATIENT_NOTE_MENTIONED requires specific note targeting.
Both PATIENT_NOTE_ADDED and PATIENT_NOTE_MENTIONED use identical URLs. Verify if the mention notification should navigate to the specific note where the user was mentioned.
✅ Verification successful
Based on the search results, I can now provide a final response:
PATIENT_NOTE_MENTIONED should navigate to the general notes section
The current implementation is correct. Here's why:
- The consultation notes are displayed in a single view at
/facility/${data.facility}/patient/${data.patient}/consultation/${data.consultation}/notes - There is no specific note targeting in the URL structure for individual notes
- The notification is meant to direct users to the consultation notes section where they can view all notes, including mentions
- The same URL pattern is used for both PATIENT_NOTE_ADDED and PATIENT_NOTE_MENTIONED, which is appropriate since they both lead to the same notes view
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's any specific handling for note mentions in the codebase
# Look for any specific handling of note mentions in components or routes
rg -A 5 "PATIENT_NOTE_MENTIONED" --type ts --type tsx
# Check for any note-specific URL parameters or routes
rg -A 5 "consultation/.*/notes/.*" --type ts --type tsx
Length of output: 167
Script:
#!/bin/bash
# Search for PATIENT_NOTE_MENTIONED without file type restrictions
rg -A 5 "PATIENT_NOTE_MENTIONED"
# Search for note-specific URL patterns without file type restrictions
rg -A 5 "consultation.*notes"
# Search for any note-specific routing or navigation logic
rg -A 5 "note.*id|noteId"
Length of output: 31678
src/components/Facility/models.tsx (3)
570-570: LGTM! Property addition aligns with profile image feature.
The new read_profile_picture_url property follows existing naming conventions and correctly supports the user profile images feature.
586-586: LGTM! Property addition supports reply feature.
The optional reply_to property correctly supports the hierarchical reply structure for doctor notes.
583-583:
Fix typo in interface name: "PaitentNotesReplyModel".
Rename the interface from "PaitentNotesReplyModel" to "PatientNotesReplyModel" to maintain consistency and correctness.
-export interface PaitentNotesReplyModel {
+export interface PatientNotesReplyModel {Likely invalid or redundant comment.
src/Redux/api.tsx (2)
824-828: LGTM: Well-structured endpoint for retrieving patient notes.
The endpoint follows RESTful conventions and is properly typed with PatientNotesModel as the response type.
Line range hint 1143-1166: LGTM: Comprehensive file upload API endpoints.
The endpoints provide complete CRUD operations with proper typing and follow RESTful conventions. This implementation supports the PR objectives for file upload functionality.
src/common/constants.tsx (1)
672-676: LGTM! The new notification event is well-structured.
The addition of the PATIENT_NOTE_MENTIONED notification event follows the existing pattern and uses appropriate values for the notification type.
src/components/Facility/ConsultationDoctorNotes/index.tsx (4)
44-47: Ensure type safety for the mode state variable.
The mode state variable is correctly typed as a union of string literals, ensuring type safety. This is a good practice to prevent unexpected values and catch potential bugs at compile time.
173-180: Verify the integration of the RichTextEditor component.
The RichTextEditor component has replaced the previous text input field, providing a more robust editing experience for notes. However, it's important to verify that the component is properly integrated and functions as expected.
To verify the integration of the RichTextEditor component, run the following script:
#!/bin/bash
# Description: Verify the integration of the `RichTextEditor` component.
# Test: Search for the usage of the `RichTextEditor` component.
# Expect: The component should be used consistently across the codebase.
rg --type tsx? --type jsx? $'RichTextEditor'184-193: Ensure proper error handling and loading states for the PatientNotesDetailedView component.
The PatientNotesDetailedView component is conditionally rendered based on the threadViewNote state. While this allows for detailed viewing of specific notes, it's important to ensure that the component handles error states and loading states gracefully.
To verify the error handling and loading states, run the following script:
#!/bin/bash
# Description: Verify error handling and loading states in the `PatientNotesDetailedView` component.
# Test: Search for the `PatientNotesDetailedView` component.
# Expect: The component should have error handling and loading states.
ast-grep --pattern $'PatientNotesDetailedView'2-18: Ensure consistent import paths and extensions.
The import statements have been updated to include .js extensions for various modules. While this improves clarity regarding file types, it's important to maintain consistency across the codebase. Verify that the same convention is followed in other files and consider updating the project's coding guidelines to reflect this change.
To verify the consistency of import paths and extensions across the codebase, run the following script:
src/components/Common/RichTextEditor.tsx (2)
159-203: Verify 'Enter' key handling allows normal line breaks
The onKeyDown handler prevents the default action of the Enter key to manage list and quote formatting. This might unintentionally prevent users from inserting line breaks in normal text. Ensure that:
- Users can insert new lines when not typing within a list or quote.
- The handler only prevents the default action when necessary for formatting purposes.
611-611:
Fix incorrect method for clearing textarea content
Using editorRef.current.innerHTML = "" will not clear the content of a <textarea> element since it does not have an innerHTML property that affects user input. Instead, you should set the value property to an empty string to clear the content:
Apply this diff to correct the issue:
- editorRef.current.innerHTML = "";
+ editorRef.current.value = "";Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx (4)
31-40: Consider refactoring StateInterface to match actual usage.The
StateInterfaceincludes properties for image manipulation (zoom, rotation) that don't appear to be used in this component. Consider moving these properties to a separate interface if they're meant for a different component, or remove them if unused.
42-54: Consider improving state management and type safety.A few suggestions to enhance the code:
- Consider consolidating pagination state into a single object.
- Use an enum or const object for tab values to prevent typos.
Example implementation:
type TabType = 'UNARCHIVED' | 'ARCHIVED'; interface PaginationState { currentPage: number; offset: number; } const [paginationState, setPaginationState] = useState<PaginationState>({ currentPage: 1, offset: 0 }); const [tab, setTab] = useState<TabType>('UNARCHIVED');
119-124: Extract permission check logic to a separate function.The permission check logic for file editability should be extracted to improve readability and reusability.
Example implementation:
const canEditFile = (uploadedByUsername: string) => { return ( uploadedByUsername === authUser.username || authUser.user_type === "DistrictAdmin" || authUser.user_type === "StateAdmin" ); }; // Usage in JSX editable={canEditFile(item?.uploaded_by?.username)}
96-147: Consider adding accessibility improvements.The component could benefit from improved accessibility:
- Add ARIA labels to interactive elements
- Ensure proper heading hierarchy
- Add keyboard navigation support for the file list
Example improvements:
-<div className={`md:p-4 ${props.className}`}> +<div className={`md:p-4 ${props.className}`} role="region" aria-label={t('discussion_notes_files')}> -<h3>Discussion Notes Files</h3> +<h2>Discussion Notes Files</h2>src/components/Common/RichTextEditor.tsx (2)
14-20: Add JSDoc documentation for the component props interface.Adding detailed documentation for each prop would improve maintainability and help other developers understand the component's API.
+/** + * Props for the RichTextEditor component + * @interface RichTextEditorProps + * @property {string} [initialMarkdown] - Initial markdown content + * @property {(markdown: string) => void} onChange - Callback when markdown content changes + * @property {() => Promise<string | undefined>} onAddNote - Callback to handle note addition + * @property {boolean} [isAuthorized] - Whether the user is authorized to edit + * @property {() => void} [onRefetch] - Callback to refetch data after changes + */ interface RichTextEditorProps { initialMarkdown?: string; onChange: (markdown: string) => void; onAddNote: () => Promise<string | undefined>; isAuthorized?: boolean; onRefetch?: () => void; }
33-39: Optimize component performance with React.memo and useCallback.The component handles complex state and renders many elements. Consider:
- Wrapping the component with React.memo
- Memoizing callback props
- Breaking down into smaller, memoized components
+const MemoizedToolbar = React.memo(({ onBold, onItalic, ... }) => { + return ( + <div className="flex items-center..."> + {/* toolbar buttons */} + </div> + ); +}); -const RichTextEditor: React.FC<RichTextEditorProps> = ({ +const RichTextEditor = React.memo<RichTextEditorProps>(({ initialMarkdown: markdown = "", onChange: setMarkdown, onAddNote, isAuthorized = true, onRefetch, -}) => { +}) => {src/components/Facility/ConsultationDoctorNotes/index.tsx (4)
49-52: Enhance Type Safety by Defining a Shared Type formodeThe
modestate is defined with a union type and initialized to"default-view". However, when updatingmodein theonTabChangehandler, you are using a type assertion (as). To improve type safety and avoid unnecessary assertions, consider defining a shared type formode.Apply the following changes:
+ type ViewMode = "thread-view" | "default-view"; - const [mode, setMode] = useState<"thread-view" | "default-view">( + const [mode, setMode] = useState<ViewMode>( "default-view", ); ... - onTabChange={(tab) => setMode(tab as "thread-view" | "default-view")} + onTabChange={(tab: ViewMode) => setMode(tab)}This ensures that
tabis correctly typed throughout your component, enhancing maintainability and reducing potential runtime errors.
Line range hint
71-91: Handle Errors When Adding a NoteIn the
onAddNotefunction, there is no error handling for unsuccessful API calls. If the request fails, the user won't receive any feedback, anddata?.idmight beundefined, which could lead to issues downstream.Consider adding error handling to notify the user of failures:
if (res?.status === 201) { Notification.Success({ msg: "Note added successfully" }); setState({ ...state, cPage: 1 }); setNoteField(""); setReload(true); setReplyTo(undefined); + return data?.id; } else { + Notification.Error({ msg: "Failed to add note. Please try again." }); + return null; }This provides feedback to the user and ensures that your function handles both success and failure cases appropriately.
33-33: Internationalize All User-Facing StringsWhile you've introduced
useTranslation, some user-facing strings are still hardcoded. Internationalizing all strings ensures consistency and allows for localization.For instance, update the
Pagetitle and tab texts:// At line 120~ - title="Discussion Notes" + title={t("discussion_notes")} // At lines 131-132 - { text: "Thread View", value: "thread-view" }, - { text: "Default View", value: "default-view" }, + { text: t("thread_view"), value: "thread-view" }, + { text: t("default_view"), value: "default-view" },Remember to add these keys to your translation files.
Line range hint
138-198: Optimize Re-renders by Memoizing ComponentsThe layout includes components like
PatientConsultationNotesListandPatientNotesDetailedViewthat could benefit from memoization to prevent unnecessary re-renders.Wrap these components with
React.memo:export default React.memo(PatientConsultationNotesList);Additionally, ensure that any functions passed as props are memoized using
useCallbackto avoid causing re-renders due to changing references.Example:
const memoizedSetReplyTo = useCallback((note) => setReplyTo(note), [setReplyTo]); // Then pass memoizedSetReplyTo instead of setReplyTo <PatientConsultationNotesList ... setReplyTo={memoizedSetReplyTo} />This improves performance, especially in complex interfaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
public/locale/en.json(1 hunks)src/Utils/request/api.tsx(3 hunks)src/common/constants.tsx(1 hunks)src/components/Common/FilePreviewDialog.tsx(1 hunks)src/components/Common/MarkdownPreview.tsx(1 hunks)src/components/Common/MentionDropdown.tsx(1 hunks)src/components/Common/RichTextEditor.tsx(1 hunks)src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx(1 hunks)src/components/Facility/ConsultationDetails/index.tsx(2 hunks)src/components/Facility/ConsultationDoctorNotes/index.tsx(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/common/constants.tsx
- src/components/Common/FilePreviewDialog.tsx
- src/components/Common/MarkdownPreview.tsx
- src/components/Common/MentionDropdown.tsx
- src/components/Facility/ConsultationDetails/index.tsx
🔇 Additional comments (5)
src/components/Common/RichTextEditor.tsx (2)
514-518: Remove redundant onChange handler.
The onChange handler is redundant as onInput already updates the markdown state. This could lead to unnecessary re-renders.
<textarea
id="discussion_notes_textarea"
ref={editorRef}
className="max-h-[300px] min-h-[70px] w-full resize-none overflow-y-auto border-none p-2 align-middle text-sm outline-none focus:outline-none focus:ring-0"
value={markdown}
- onChange={(e) => {
- setMarkdown(e.target.value);
- }}
onInput={handleInput}
onKeyDown={onKeyDown}
/>504-507:
Ensure markdown rendering is secure against XSS attacks.
The MarkdownPreview component should sanitize the markdown content before rendering to prevent XSS attacks.
src/Utils/request/api.tsx (3)
814-818: LGTM! Well-structured endpoint definition.
The getPatientNote endpoint follows RESTful conventions and maintains consistency with other patient note endpoints.
Line range hint 1128-1151: LGTM! Comprehensive file upload endpoints.
The FileUploads section provides well-structured CRUD operations with proper type safety and error handling.
1152-1158: LGTM! Verify consultation file upload integration.
The consultation file upload endpoint aligns with the PR objectives for enhancing file upload capabilities.
✅ Verification successful
Consultation file upload integration verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consultation file upload endpoint integration
# Test: Search for components using the listConsultationFileUploads endpoint
# Expect: Usage in consultation/patient note related components
# Search for direct usage of the endpoint
rg "listConsultationFileUploads" -t tsx -t ts
# Search for components handling consultation files
ast-grep --pattern 'import { FileUploadModel } from "@/components/Patient/models"'
Length of output: 683
Script:
#!/bin/bash
# Description: Verify the consultation file upload endpoint integration with corrected ripgrep usage
# Test: Search for components using the listConsultationFileUploads endpoint
# Expect: Usage in consultation/patient note related components
# Search for direct usage of the endpoint in .ts and .tsx files
rg "listConsultationFileUploads" -g "*.ts" -g "*.tsx"
# Search for components handling consultation files
ast-grep --pattern 'import { FileUploadModel } from "@/components/Patient/models"'
Length of output: 1016
src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx
Outdated
Show resolved
Hide resolved
src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx
Outdated
Show resolved
Hide resolved
src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
src/components/Common/MarkdownPreview.tsx (2)
12-25: Enhance UserCard accessibility and edge case handling.Consider these improvements for the UserCard component:
- Add ARIA labels for better screen reader support
- Handle edge cases where first_name might be empty
const UserCard = ({ user }: { user: UserBareMinimum }) => ( - <div className="z-10 flex w-64 items-center space-x-3 rounded-lg bg-gray-200 px-3 pb-3 shadow-lg"> + <div + className="z-10 flex w-64 items-center space-x-3 rounded-lg bg-gray-200 px-3 pb-3 shadow-lg" + role="tooltip" + aria-label={`User details for ${user.first_name} ${user.last_name}`} + > <div className="flex h-10 w-10 items-center justify-center rounded-full bg-primary text-lg font-semibold text-white"> - {user.first_name[0]} + {user.first_name?.[0] || user.username[0]} </div>
87-97: Optimize component performance with memoization.Consider memoizing the component to prevent unnecessary re-renders, especially when handling large markdown content.
-const MarkdownPreview = ({ +const MarkdownPreview = React.memo(({ markdown, mentioned_users, }: { markdown: string; mentioned_users?: UserBareMinimum[]; -}) => { +}) => { // ... component implementation ... -}; +});Also consider memoizing the processed markdown if the content is large:
+ const processedMarkdownMemo = React.useMemo( + () => markdown + .replace(/@([a-zA-Z0-9_]{3,30})/g, ...) + .replace(/~~(.*?)~~/g, ...), + [markdown, mentioned_users] + );src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx (3)
24-42: Add JSDoc documentation and consider property validation.The interfaces
ModalDetailsandStateInterfacewould benefit from:
- JSDoc documentation explaining their purpose and usage
- Required vs optional properties validation for
ModalDetails- Type constraints for numeric properties in
StateInterfaceExample implementation:
/** * Represents the details required for file modal operations * @property name - The name of the file * ... */ export interface ModalDetails { name: string; // Make required if always needed id: string; // Make required if always needed reason?: string; userArchived?: string; archiveTime?: string; associatedId: string; // Make required if always needed } /** * Manages the state of file preview functionality * @property zoom - Zoom level between 0.1 and 5.0 * @property rotation - Rotation angle in degrees (0-360) */ export interface StateInterface { open: boolean; isImage: boolean; name: string; extension: string; zoom: number; // Add constraints: 0.1 <= zoom <= 5.0 isZoomInDisabled: boolean; isZoomOutDisabled: boolean; rotation: number; // Add constraints: 0 <= rotation < 360 }
51-55: Enhance pagination logic with validation and error handling.The pagination logic could be improved by:
- Validating input parameters
- Handling edge cases
- Using URL parameters for bookmarkable pages
Example implementation:
const handlePagination = (page: number, limit: number) => { try { if (page < 1) throw new Error('Page must be positive'); if (limit !== RESULTS_PER_PAGE_LIMIT) { throw new Error('Invalid page limit'); } const offset = (page - 1) * limit; setCurrentPage(page); setOffset(offset); // Update URL for bookmarkable pages const searchParams = new URLSearchParams(window.location.search); searchParams.set('page', page.toString()); window.history.replaceState(null, '', `?${searchParams.toString()}`); } catch (error) { console.error('Pagination error:', error); // Reset to first page setCurrentPage(1); setOffset(0); } };
110-112: Enhance loading state UI with better skeleton loading.The current loading state could be improved to better match the actual content layout.
Example implementation:
const LoadingSkeleton = () => ( <> {[1, 2, 3].map((key) => ( <div key={key} className="flex animate-pulse gap-4 rounded-lg bg-gray-100 p-4"> <div className="h-12 w-12 rounded-full bg-gray-200" /> <div className="flex-1 space-y-2"> <div className="h-4 w-3/4 rounded bg-gray-200" /> <div className="h-4 w-1/2 rounded bg-gray-200" /> </div> </div> ))} </> ); // In component: {!(fileQuery?.data?.results || []).length && loading && <LoadingSkeleton />}src/components/Facility/ConsultationDoctorNotes/index.tsx (2)
Line range hint
64-92: Enhance error handling and i18n support in onAddNoteThe function has several areas for improvement:
- The error message should use the translation system
- The function should handle API failures explicitly
- The note validation could be more comprehensive
Consider applying these changes:
const onAddNote = async () => { if (!/\S+/.test(noteField)) { Notification.Error({ - msg: "Note Should Contain At Least 1 Character", + msg: t('error_messages.note_empty'), }); return; } const { res, data } = await request(routes.addPatientNote, { pathParams: { patientId: patientId, }, body: { note: noteField, thread, consultation: consultationId, reply_to: reply_to?.id, }, }); if (res?.status === 201) { Notification.Success({ msg: t('success_messages.note_added') }); setState({ ...state, cPage: 1 }); setNoteField(""); setReload(true); setReplyTo(undefined); + return data?.id; } - return data?.id; + Notification.Error({ msg: t('error_messages.note_add_failed') }); + return undefined; };
128-138: LGTM: Well-structured responsive layoutThe UI implementation is clean and follows responsive design best practices. The conditional rendering for thread view and the tab-based navigation are well-implemented.
Consider adding loading states to improve user experience during view transitions:
<div className="flex flex-1 overflow-hidden"> <div className={classNames( "flex flex-1 flex-col", threadViewNote && "max-sm:hidden", + isLoading && "opacity-50" )} >Also applies to: 157-199
src/components/Facility/PatientNotesSlideover.tsx (2)
Line range hint
37-86: Consider consolidating state managementThe component manages multiple pieces of state (thread, show, patientActive, reload, reply_to, noteField) which increases complexity and makes the component harder to maintain. Consider:
- Using useReducer for related state transitions
- Extracting notification logic into a custom hook
- Creating a dedicated hook for note field persistence
Example refactor for the notification state:
// useNotificationState.ts export function useNotificationState() { const notificationState = useNotificationSubscriptionState(); useEffect(() => { if (notificationState === "unsubscribed") { Notification.Warn({ msg: "Please subscribe to notifications to get live updates on discussion notes." }); } else if (notificationState === "subscribed_on_other_device") { Notification.Warn({ msg: "Please subscribe to notifications on this device to get live updates on discussion notes." }); } }, [notificationState]); return notificationState; }
Line range hint
87-103: Enhance error handling and validationThe current note addition logic has several areas for improvement:
- The empty note validation is basic and doesn't handle whitespace-only content properly
- No validation of markdown content
- No comprehensive error handling for API failures
Consider applying these improvements:
const onAddNote = async () => { - if (!/\S+/.test(noteField)) { + const trimmedNote = noteField.trim(); + if (!trimmedNote) { Notification.Error({ msg: "Note Should Contain At Least 1 Character", }); return; } + try { const { res, data } = await request(routes.addPatientNote, { pathParams: { patientId: patientId }, body: { - note: noteField, + note: trimmedNote, consultation: consultationId, thread, reply_to: reply_to?.id, }, }); if (res?.status === 201) { Notification.Success({ msg: "Note added successfully" }); setNoteField(""); setState({ ...state, cPage: 1 }); setReload(true); setReplyTo(undefined); return data?.id; } + throw new Error('Failed to add note'); + } catch (error) { + Notification.Error({ + msg: error.message || "Failed to add note. Please try again.", + }); + return undefined; + } };src/components/Facility/PatientNotesDetailedView.tsx (1)
37-38: Improve error message capitalizationFor better readability and consistency, the error message should follow standard sentence capitalization.
Apply this diff:
Notification.Error({ - msg: "Note Should Contain At Least 1 Character", + msg: "Note should contain at least 1 character", });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.json(1 hunks)src/components/Common/MarkdownPreview.tsx(1 hunks)src/components/Common/MentionDropdown.tsx(1 hunks)src/components/Common/RichTextEditor.tsx(1 hunks)src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx(1 hunks)src/components/Facility/ConsultationDoctorNotes/index.tsx(7 hunks)src/components/Facility/PatientNotesDetailedView.tsx(1 hunks)src/components/Facility/PatientNotesSlideover.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Common/MentionDropdown.tsx
🔇 Additional comments (8)
package.json (1)
94-94: LGTM! Dependencies are up-to-date and secure.
The added markdown-related dependencies are using their latest stable versions and are well-maintained.
Let's verify the bundle size impact and ensure all required plugins are included:
Also applies to: 97-97
src/components/Facility/ConsultationDoctorNotes/index.tsx (2)
2-2: LGTM: Clean state management and i18n setup
The new state management for thread view and the integration of i18n translations are well-implemented. The TypeScript types are properly defined for the mode state.
Also applies to: 5-6, 50-53
129-137:
Address accessibility concerns in tab navigation
The previous review comment about accessibility is still valid. Additionally, consider adding keyboard navigation support:
Add keyboard event handlers:
<Tabs
className="mt-1 w-full gap-8 lg:w-full"
+ onKeyDown={(e) => {
+ if (e.key === 'ArrowRight') {
+ setMode('thread-view');
+ } else if (e.key === 'ArrowLeft') {
+ setMode('default-view');
+ }
+ }}
tabs={[
{ text: "Thread View", value: "thread-view" },
{ text: "Default View", value: "default-view" },
]}
currentTab={mode}
onTabChange={(tab) => setMode(tab as "thread-view" | "default-view")}
/>src/components/Facility/PatientNotesSlideover.tsx (1)
241-258: Ensure rich text editor meets accessibility standards
The new rich text editor integration should be tested for accessibility compliance:
- Keyboard navigation
- Screen reader compatibility
- ARIA labels and roles
- Color contrast for markdown syntax
Consider implementing the following accessibility enhancements:
- Add aria-label to the editor
- Ensure proper focus management when switching between reply and edit modes
- Add keyboard shortcuts for common formatting actions
- Include screen reader announcements for state changes
src/components/Common/RichTextEditor.tsx (4)
515-520: 🛠️ Refactor suggestion
Optimize state updates in event handlers
The onChange handler updates setMarkdown directly while handleInput (attached to onInput) also updates setMarkdown. This leads to redundant state updates.
- onChange={(e) => {
- setMarkdown(e.target.value);
- }}
+ onChange={handleInput}
onInput={handleInput}
onKeyDown={onKeyDown}Likely invalid or redundant comment.
609-618:
Add error handling for async operations
The async operations lack proper error handling, which could lead to a poor user experience if operations fail.
onClick={async () => {
if (!editorRef.current) return;
+ try {
const id = await onAddNote();
if (!id) return;
await fileUpload.handleFileUpload(id);
onRefetch?.();
fileUpload.clearFiles();
editorRef.current.innerHTML = "";
setIsPreviewMode(false);
+ } catch (error) {
+ console.error('Failed to add note:', error);
+ // TODO: Add user notification for failure
+ }
}}Likely invalid or redundant comment.
407-413: 🛠️ Refactor suggestion
Enhance accessibility with ARIA labels
Interactive elements lack proper accessibility attributes:
- Missing
aria-labelattributes - No keyboard focus indicators
- Missing role attributes
<button
onClick={() => insertMarkdown("**")}
- className="tooltip rounded bg-gray-200/50 p-1"
+ className="tooltip rounded bg-gray-200/50 p-1 focus:ring-2 focus:ring-primary-500"
+ aria-label="Bold text"
+ role="button"
>
<CareIcon icon="l-bold" className="text-lg" />
<span className="tooltip-text tooltip-top -translate-x-4">Bold</span>
</button>Likely invalid or redundant comment.
505-509: Verify XSS protection in Markdown preview
When rendering user-generated Markdown content, ensure proper XSS protection is in place.
src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx
Outdated
Show resolved
Hide resolved
src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (16)
src/components/Facility/DoctorNoteReplyPreviewCard.tsx (1)
71-72: Consider improving content truncation handling.The
max-h-14class withoverflow-hiddenmight abruptly cut off content mid-word or mid-line. Consider adding a fade-out effect or "show more" button for better UX.- <div className="max-h-14 overflow-hidden pb-2 pl-14 text-sm text-gray-700"> + <div className="relative max-h-14 overflow-hidden pb-2 pl-14 text-sm text-gray-700"> <MarkdownPreview markdown={parentNote.note} /> + <div className="absolute bottom-0 h-4 w-full bg-gradient-to-t from-gray-200/50 to-transparent"></div> </div>src/components/Facility/DoctorNote.tsx (2)
Line range hint
11-18: Improve type safety by properly typing thesetReloadprop.The
setReloadprop is currently typed asany, which bypasses TypeScript's type checking. Consider defining a more specific type for better type safety.interface DoctorNoteProps { state: PatientNoteStateType; - setReload: any; + setReload: () => void; handleNext: () => void; disableEdit?: boolean; setReplyTo?: (reply_to: PatientNotesModel | undefined) => void; mode?: "thread-view" | "default-view"; setThreadViewNote?: (noteId: string) => void; }
34-34: Consider using responsive height instead of fixed height.The fixed height of 500px might not work well across different screen sizes and devices. Consider using viewport units or a more responsive approach.
- className="mt-4 flex h-[500px] grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2" + className="mt-4 flex h-[80vh] grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2"Also applies to: 46-46
src/components/Common/MarkdownPreview.tsx (2)
7-20: Enhance accessibility of UserCard componentThe component should include ARIA labels and roles for better screen reader support.
-const UserCard = ({ user }: { user: UserBareMinimum }) => ( +const UserCard = ({ user }: { user: UserBareMinimum }) => ( - <div className="z-10 flex w-64 items-center space-x-3 rounded-lg bg-gray-200 px-3 pb-3 shadow-lg"> + <div + role="tooltip" + aria-label={`Profile card for ${user.first_name} ${user.last_name}`} + className="z-10 flex w-64 items-center space-x-3 rounded-lg bg-gray-200 px-3 pb-3 shadow-lg" + > <div className="flex h-10 w-10 items-center justify-center rounded-full bg-primary text-lg font-semibold text-white"> {user.first_name[0]} </div>
22-31: Optimize performance and enhance type safetyConsider these improvements:
- Add input validation for markdown content
- Memoize the MentionedUsers object to prevent unnecessary recalculations
+import { useMemo } from "react"; + const MarkdownPreview = ({ markdown, mentioned_users, }: { - markdown: string; + markdown: string | undefined; mentioned_users?: UserBareMinimum[]; }) => { - const MentionedUsers = Object.fromEntries( - mentioned_users?.map((u) => [u.username, u]) ?? [], + const MentionedUsers = useMemo( + () => Object.fromEntries(mentioned_users?.map((u) => [u.username, u]) ?? []), + [mentioned_users] ); + + if (!markdown) return null;src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx (3)
25-32: Consider adding validation for modal detailsThe
ModalDetailsinterface has all optional fields which could lead to runtime issues if required fields are missing.Consider marking required fields as non-optional and adding runtime validation:
export interface ModalDetails { id: string; // Required name: string; // Required reason?: string; userArchived?: string; archiveTime?: string; associatedId: string; // Required if used for file association }
52-57: Optimize pagination handler with useCallbackConsider memoizing the pagination handler to prevent unnecessary re-renders.
const handlePagination = useCallback((page: number, limit: number) => { const offset = (page - 1) * limit; setCurrentPage(page); setOffset(offset); }, []);
99-108: Enhance error handling in refetchAllThe error message could be more specific and include error details for better debugging.
const refetchAll = async () => Promise.all( Object.values(queries).map((q) => q.refetch?.().catch((error) => { console.error('File refetch failed:', error); Notification.Error({ msg: `Failed to refetch ${error.response?.data?.detail || 'files'}`, }); }), ), );src/components/Patient/PatientNotes.tsx (1)
81-86: Consider enhancing error messages with more context.While the error handling structure is good, consider making the error messages more specific based on the error type:
- throw new Error("Failed to add note"); + throw new Error(`Failed to add note: ${res.statusText}`); } } catch (error) { Notification.Error({ - msg: "Failed to add note. Please try again.", + msg: `Failed to add note: ${error instanceof Error ? error.message : 'Unknown error occurred'}. Please try again.`, });src/components/Facility/ConsultationDoctorNotes/index.tsx (3)
50-52: Consider using a const enum or union literal type for view modesTo improve type safety and maintainability, consider defining the view modes as a const enum or union literal type.
+type ViewMode = 'thread-view' | 'default-view'; +const VIEW_MODES = { + THREAD: 'thread-view' as ViewMode, + DEFAULT: 'default-view' as ViewMode, +} as const; -const [mode, setMode] = useState<"thread-view" | "default-view">("default-view"); +const [mode, setMode] = useState<ViewMode>(VIEW_MODES.DEFAULT);
Line range hint
72-92: Enhance error handling in onAddNote functionThe function should handle API errors explicitly and provide appropriate error messages to users.
const { res, data } = await request(routes.addPatientNote, { pathParams: { patientId: patientId, }, body: { note: noteField, thread, consultation: consultationId, reply_to: reply_to?.id, }, }); -if (res?.status === 201) { +if (res?.status === 201 && data?.id) { Notification.Success({ msg: "Note added successfully" }); setState({ ...state, cPage: 1 }); setNoteField(""); setReload(true); setReplyTo(undefined); +} else { + Notification.Error({ + msg: "Failed to add note. Please try again." + }); + return undefined; } return data?.id;
179-186: Consider wrapping RichTextEditor with error boundarySince the RichTextEditor is a complex component handling markdown and user input, it would be beneficial to wrap it with an error boundary to gracefully handle runtime errors.
+import ErrorBoundary from '@/components/Common/ErrorBoundary'; +<ErrorBoundary fallback={<div>Failed to load editor. Please refresh the page.</div>}> <RichTextEditor initialMarkdown={noteField} onChange={setNoteField} onAddNote={onAddNote} isAuthorized={patientActive} onRefetch={() => setReload(true)} maxRows={10} /> +</ErrorBoundary>src/components/Facility/PatientNotesSlideover.tsx (1)
Line range hint
87-103: Consider adding type safety for the return valueWhile the function correctly handles the reply functionality, consider adding type safety for the return value to handle cases where
datamight be undefined.- return data?.id; + return data?.id ?? null;src/components/Facility/PatientNoteCard.tsx (3)
213-213: Use optional chaining for better code readabilityReplace the conditional check with optional chaining.
Apply this diff:
-setReplyTo && setReplyTo(note); +setReplyTo?.(note);🧰 Tools
🪛 Biome
[error] 213-213: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
141-156: Enhance note validationThe current validation only checks for non-empty content. Consider adding more comprehensive validation for markdown content and maximum length.
Apply this diff:
const onUpdateNote = async () => { if (noteField === note.note) { setIsEditing(false); return; } const payload = { note: noteField, }; - if (!/\S+/.test(noteField)) { + const trimmedNote = noteField.trim(); + if (!trimmedNote) { Error({ - msg: "Note Should Contain At Least 1 Character", + msg: "Note cannot be empty", }); return; } + if (trimmedNote.length > 5000) { // Adjust max length as needed + Error({ + msg: "Note exceeds maximum length of 5000 characters", + }); + return; + }
307-328: Add loading state for file previewsThe file preview UI should show a loading state while fetching the file to improve user experience.
Apply this diff:
+const [loadingFileId, setLoadingFileId] = useState<string | null>(null); const loadFile = async (id: string, noteId: string) => { + setLoadingFileId(id); try { // ... existing code ... } catch (error) { // ... error handling ... } finally { + setLoadingFileId(null); } }; // In the render method: {note?.files?.map((file: FileUploadModel) => ( <div key={file.id} className="relative mt-1 h-20 w-20 cursor-pointer rounded-md bg-gray-100 shadow-sm hover:bg-gray-200" > <div className="flex h-full w-full flex-col items-center justify-center p-2" onClick={(e) => { e.stopPropagation(); if (file.id) loadFile(file.id, note.id); }} > + {loadingFileId === file.id ? ( + <Spinner size="small" /> + ) : ( <CareIcon icon="l-file" className="shrink-0 text-2xl text-gray-600" /> + )} <span className="mt-1 max-h-[2.5em] w-full overflow-hidden text-ellipsis break-words text-center text-xs text-gray-600"> {file.name} </span> </div> </div> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
src/components/Common/MarkdownPreview.tsx(1 hunks)src/components/Common/MentionDropdown.tsx(1 hunks)src/components/Common/RichTextEditor.tsx(1 hunks)src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx(1 hunks)src/components/Facility/ConsultationDoctorNotes/index.tsx(7 hunks)src/components/Facility/DoctorNote.tsx(2 hunks)src/components/Facility/DoctorNoteReplyPreviewCard.tsx(2 hunks)src/components/Facility/PatientNoteCard.tsx(6 hunks)src/components/Facility/PatientNotesDetailedView.tsx(1 hunks)src/components/Facility/PatientNotesSlideover.tsx(8 hunks)src/components/Facility/models.tsx(3 hunks)src/components/Patient/PatientNotes.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Common/MentionDropdown.tsx
- src/components/Facility/PatientNotesDetailedView.tsx
🧰 Additional context used
🪛 Biome
src/components/Common/MarkdownPreview.tsx
[error] 88-88: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
src/components/Facility/DoctorNote.tsx
[error] 53-62: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
src/components/Facility/PatientNoteCard.tsx
[error] 213-213: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (22)
src/components/Facility/DoctorNoteReplyPreviewCard.tsx (2)
3-16: LGTM! Well-structured imports and interface definition.
The Props interface correctly handles optional types, and the imports are properly organized.
23-25: LGTM! Clean early return pattern.
Good use of early return pattern to handle undefined parentNote prop, making the code more maintainable.
src/components/Facility/DoctorNote.tsx (1)
22-30: LGTM!
Props destructuring is clean and well-organized.
src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx (3)
45-51: LGTM! Clean initialization and state management
The component initialization follows React best practices with proper use of hooks and state management.
58-98: Skipping duplicate comments about query implementation
Previous reviews have already addressed the need to consolidate duplicate query logic and add error handling.
34-43: Clarify the purpose of image-specific state properties
The StateInterface includes image-specific properties (zoom, rotation, etc.) but the component name suggests it's for discussion notes files.
Let's verify if these properties are actually used:
✅ Verification successful
Image-specific properties are correctly used for file preview functionality
The StateInterface properties (zoom, rotation, etc.) are used by the FilePreviewDialog component to handle image preview functionality. These properties are managed by the useFileManager hook and are essential for:
- Controlling image zoom levels through
zoomproperty - Managing image rotation through
rotationproperty - Handling zoom limits through
isZoomInDisabledandisZoomOutDisabled
The component name "DiscussionNotesFilesTab" is appropriate as it handles both image and non-image files, with these properties being utilized specifically when previewing image files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of zoom and rotation properties
rg -A 2 "zoom|rotation|isZoomInDisabled|isZoomOutDisabled" --type typescript
Length of output: 113
Script:
#!/bin/bash
# Search for usage of zoom and rotation properties without file type restriction
rg -A 2 "zoom|rotation|isZoomInDisabled|isZoomOutDisabled"
# Also search for the component name to understand its usage
rg -A 5 "DiscussionNotesFilesTab"
Length of output: 12533
Script:
#!/bin/bash
# Let's check the implementation of FilePreviewDialog since it seems to be handling these properties
rg -A 10 "FilePreviewDialog" --type tsx
# Also check if DiscussionNotesFilesTab uses FilePreviewDialog
cat src/components/Facility/ConsultationDetails/DiscussionNotesFilesTab.tsx
Length of output: 5105
Script:
#!/bin/bash
# Let's check the FileBlock component since it's being used in DiscussionNotesFilesTab
cat src/components/Files/FileBlock.tsx
# Also check the useFileManager hook implementation
cat src/hooks/useFileManager.tsx
Length of output: 18877
src/components/Patient/PatientNotes.tsx (5)
28-29: LGTM! Type safety improvements.
Good job improving type safety by replacing any types with explicit string types for patientId and facilityId.
34-34: LGTM! Proper i18n implementation.
Good use of the useTranslation hook for internationalization support.
47-47: LGTM! Clean state initialization.
The state initialization for reply_to is clean and properly typed with PatientNotesModel.
129-130: Verify color scheme consistency.
The color scheme has been updated from secondary-* to gray-*. Let's verify this aligns with the design system used across the application.
#!/bin/bash
# Search for other instances of secondary-300 and secondary-200 colors
# to ensure consistent updates across the codebase
echo "Checking for remaining secondary-300 usage:"
rg "secondary-300" --type tsx
echo "Checking for remaining secondary-200 usage:"
rg "secondary-200" --type tsx
echo "Checking for new gray-300 usage:"
rg "gray-300" --type tsx
echo "Checking for new gray-200 usage:"
rg "gray-200" --type tsx64-87: Verify reply feature functionality.
Since this PR introduces a reply feature for doctor notes, let's verify that the reply_to functionality is working as expected:
- The note ID is correctly returned on successful creation
- The reply is properly linked to the parent note
- The UI updates appropriately after adding a reply
✅ Verification successful
Let me gather more information about the reply feature implementation since the previous search didn't find the routes file.
Based on the gathered information, I can now provide a final response since we have comprehensive evidence about the reply feature implementation.
Reply feature implementation is properly structured and consistent
The implementation shows a well-structured reply feature with:
- Proper API integration with
reply_tofield in request payload - Consistent state management across components using
reply_toandsetReplyTo - Complete reply feature implementation in multiple views:
- PatientNotes.tsx
- PatientNotesSlideover.tsx
- ConsultationDoctorNotes/index.tsx
- PatientNotesDetailedView.tsx
- Proper cleanup of reply state (setting to undefined after submission)
- End-to-end test coverage in PatientDoctorNotes.cy.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for reply-related functionality implementation
# to ensure consistent behavior
# Check for reply_to usage in API routes
echo "Checking API routes for reply_to handling:"
rg "reply_to" --type ts routes.ts
# Check for reply-related components
echo "Checking for reply components:"
ast-grep --pattern 'const $_ = ($props: $_Props) => {
$$$
reply_to
$$$
}'
# Check for reply-related tests
echo "Checking for reply feature tests:"
fd -e cy.ts -e spec.ts -e test.ts | xargs rg "reply"
Length of output: 717
Script:
#!/bin/bash
# First, let's locate the routes file
echo "Finding routes file:"
fd routes
# Check for reply_to handling in API calls
echo "\nChecking API integration:"
rg "reply_to" --type ts
# Check the test implementation details
echo "\nChecking test implementation:"
rg -A 5 "patientNurseReplyNote" cypress/e2e/patient_spec/PatientDoctorNotes.cy.ts
# Look for reply-related state management
echo "\nChecking reply state management:"
ast-grep --pattern 'setReplyTo($$$)'
Length of output: 4438
src/components/Facility/ConsultationDoctorNotes/index.tsx (2)
12-12: LGTM: Fixed typo in type name
The type name has been corrected from PaitentNotesReplyModel to PatientNotesReplyModel.
157-163: LGTM: Well-implemented responsive layout
The responsive layout implementation using classNames utility and conditional hiding based on viewport size is well done.
src/components/Facility/PatientNotesSlideover.tsx (3)
Line range hint 3-27: LGTM: Import changes align with new features
The changes properly set up dependencies for the new rich text editor and authorization features while fixing the type name from PaitentNotesReplyModel to PatientNotesReplyModel.
Line range hint 37-47: LGTM: State management properly handles new features
The addition of translation support and reply state management is well-implemented with proper typing.
197-198: Verify the increased width accommodates all UI elements
The width increase from 400px to 430px should be tested to ensure it properly accommodates the rich text editor and its toolbar on different screen sizes.
#!/bin/bash
# Search for other components using similar width classes to ensure consistency
rg -g '*.tsx' -g '*.jsx' 'w-\[4[0-9]{2}px\]'src/components/Facility/PatientNoteCard.tsx (1)
39-52: LGTM: Props are well-typed with appropriate defaults
The new props are properly typed and include sensible default values, enhancing the component's flexibility.
src/components/Facility/models.tsx (2)
575-575: LGTM: Profile picture URL property follows conventions
The read_profile_picture_url property is correctly typed and follows the existing naming convention with the "read_" prefix.
585-591: LGTM: Interface renamed and reply feature added
Good improvements:
- Fixed the typo in the interface name (Paitent → Patient)
- Added support for nested replies with the optional
reply_toproperty
src/components/Common/RichTextEditor.tsx (3)
57-62: Skip comment: File upload security concerns already addressed in previous review.
642-651: Skip comment: Error handling in submit handler already addressed in previous review.
440-446: Skip comment: Accessibility improvements already addressed in previous review.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
src/components/Facility/DoctorNote.tsx (2)
34-34: Consider using dynamic height instead of hardcoded valueThe hardcoded height of
500pxmight not be suitable for all screen sizes and could cause layout issues.Consider using CSS variables or dynamic height calculation:
-className="mt-4 flex h-[500px] grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2" +className="mt-4 flex h-[var(--notes-container-height,500px)] grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2"
Line range hint
1-91: Consider architectural improvements for future scalabilityAs this component is part of a larger doctor notes enhancement feature, consider these architectural improvements:
- Extract the note rendering logic into a separate component to improve maintainability
- Implement a context for sharing note-related state and callbacks
- Consider using a virtual list component for better performance with large datasets
This will make it easier to add future enhancements like markdown support and file uploads.
🧰 Tools
🪛 Biome
[error] 53-62: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
src/components/Common/MarkdownPreview.tsx (2)
29-31: Optimize performance with useMemo for MentionedUsers mapThe MentionedUsers map is recreated on every render, which is inefficient.
+import { useEffect, useState, useMemo } from "react"; - const MentionedUsers = Object.fromEntries( + const MentionedUsers = useMemo(() => Object.fromEntries( mentioned_users?.map((u) => [u.username, u]) ?? [], - ); + ), [mentioned_users]);
77-118: Improve event handling with React refs and debouncingConsider using React refs and debouncing for better performance and React idioms.
+import { useEffect, useState, useMemo, useRef, useCallback } from "react"; +import debounce from 'lodash/debounce'; const MarkdownPreview = ({ ... }) => { + const containerRef = useRef<HTMLDivElement>(null); + + const handleHover = useCallback( + debounce((username: string | null, rect?: DOMRect) => { + setHoveredUser(username); + setHoverPosition(rect ? { x: rect.left, y: rect.top } : null); + }, 100), + [] + ); useEffect(() => { - const mentionElements = document.querySelectorAll(".mention"); + const mentionElements = containerRef.current?.querySelectorAll(".mention") ?? []; const listeners: Array<{ element: Element; enter: () => void; leave: () => void }> = []; mentionElements.forEach((ele) => { const handleEnter = () => { const username = ele.getAttribute("data-username"); if (username) { - setHoveredUser(username); - const rect = ele.getBoundingClientRect(); - setHoverPosition({ - x: rect.left, - y: rect.top, - }); + handleHover(username, ele.getBoundingClientRect()); } }; const handleLeave = () => { - setHoveredUser(null); - setHoverPosition(null); + handleHover(null); }; // ... rest of the code }); - }, [sanitizedHtml]); + }, [sanitizedHtml, handleHover]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Common/MarkdownPreview.tsx(1 hunks)src/components/Common/RichTextEditor.tsx(1 hunks)src/components/Facility/DoctorNote.tsx(2 hunks)
🧰 Additional context used
🪛 Biome
src/components/Common/MarkdownPreview.tsx
[error] 122-122: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
src/components/Facility/DoctorNote.tsx
[error] 53-62: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (8)
src/components/Facility/DoctorNote.tsx (2)
51-80: Well-structured rendering logic with proper key usage
The refactored code successfully:
- Extracts common note card props for reusability
- Handles different modes appropriately
- Uses keys correctly on the outermost elements
🧰 Tools
🪛 Biome
[error] 53-62: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
46-49: Review scroll performance with inverse InfiniteScroll
Using flex-col-reverse with inverse={true} on InfiniteScroll might impact performance with large lists, as it requires repositioning all items when new items are loaded.
Let's verify the impact:
src/components/Common/RichTextEditor.tsx (6)
89-97: 🛠️ Refactor suggestion
Replace setTimeout with useEffect for cursor position updates
Using setTimeout for cursor position updates could lead to race conditions with rapid markdown insertions.
Consider using useEffect to handle cursor position updates:
- setTimeout(() => {
+ useEffect(() => {
if (editorRef.current) {
editorRef.current.focus();
editorRef.current.setSelectionRange(
newCursorPosition,
newCursorPosition,
);
}
- }, 0);
+ }, [newCursorPosition]);Likely invalid or redundant comment.
57-62:
Add file upload restrictions for security
The current file upload configuration lacks important security measures that could expose the system to risks.
Apply this diff to add security restrictions:
const fileUpload = useFileUpload({
type: "NOTES",
category: "UNSPECIFIED",
multiple: true,
- allowAllExtensions: true,
+ allowAllExtensions: false,
+ maxFileSize: 5 * 1024 * 1024, // 5MB limit
+ allowedFileTypes: [
+ 'image/*',
+ 'audio/*',
+ 'application/pdf',
+ 'text/plain'
+ ],
});Likely invalid or redundant comment.
642-651:
Add error handling for async operations
The async operations in the submit handler lack proper error handling.
Apply this diff to add error handling:
onClick={async () => {
if (!editorRef.current) return;
- const id = await onAddNote();
- if (!id) return;
- await fileUpload.handleFileUpload(id);
- onRefetch?.();
- fileUpload.clearFiles();
- editorRef.current.innerHTML = "";
- setIsPreviewMode(false);
+ try {
+ const id = await onAddNote();
+ if (!id) return;
+ await fileUpload.handleFileUpload(id);
+ onRefetch?.();
+ fileUpload.clearFiles();
+ editorRef.current.innerHTML = "";
+ setIsPreviewMode(false);
+ } catch (error) {
+ console.error('Failed to add note:', error);
+ // TODO: Add error notification
+ }
}}Likely invalid or redundant comment.
440-446: 🛠️ Refactor suggestion
Enhance accessibility with ARIA labels
Interactive elements like buttons lack proper ARIA labels for screen readers.
Apply this diff to improve accessibility:
<button
onClick={() => insertMarkdown("**")}
- className="tooltip rounded bg-gray-200/50 p-1"
+ className="tooltip rounded bg-gray-200/50 p-1 focus:ring-2 focus:ring-primary-500"
+ aria-label="Bold text"
+ role="button"
>
<CareIcon icon="l-bold" className="text-lg" />
<span className="tooltip-text tooltip-top -translate-x-4">Bold</span>
</button>Likely invalid or redundant comment.
380-385:
Enhance URL validation security
The URL formatting lacks proper validation against malicious URLs.
Apply this diff to add security measures:
const formatUrl = (url: string) => {
+ const sanitizedUrl = url.trim().toLowerCase();
+
+ // Check for dangerous protocols
+ if (/^(javascript|data|vbscript):/i.test(sanitizedUrl)) {
+ throw new Error('Invalid URL protocol');
+ }
+
+ // Validate URL pattern
+ const urlPattern = /^(https?:\/\/)?([\da-z.-]+)\.([a-z.]{2,6})([/\w .-]*)*\/?$/;
+ if (!urlPattern.test(sanitizedUrl)) {
+ throw new Error('Invalid URL format');
+ }
+
if (!/^https?:\/\//i.test(url)) {
return `https://${url}`;
}
return url;
};Likely invalid or redundant comment.
538-542: Ensure Markdown rendering is secure against XSS attacks
When rendering user-generated Markdown content in MarkdownPreview, it's crucial to prevent Cross-Site Scripting (XSS) vulnerabilities.
Run the following script to check if the MarkdownPreview component implements security measures:
✅ Verification successful
XSS protection is properly implemented
The MarkdownPreview component already implements proper XSS protection through:
- Usage of DOMPurify.sanitize() on line 73 before rendering HTML content
- Proper configuration of DOMPurify with safe attributes (target, rel) for links
- Secure markdown parsing using the marked library with a custom renderer
- Safe handling of user mentions with proper HTML escaping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if MarkdownPreview component implements security measures
# Search for sanitization or security-related code in MarkdownPreview
ast-grep --pattern 'function MarkdownPreview($$$) {
$$$
sanitize($$$)
$$$
}'
# Check for usage of secure markdown libraries
rg -l "sanitize-html|dompurify|xss|marked" --type ts --type tsx
Length of output: 178
Script:
#!/bin/bash
# Let's try a different approach to find the MarkdownPreview component and its implementation
# Find the MarkdownPreview component file
fd "MarkdownPreview" --type f
# Search for markdown-related imports and security measures
rg -l "marked|sanitize|xss|dompurify" --type typescript --type javascriptreact --type typescriptreact
# Look for the actual implementation of MarkdownPreview
ast-grep --pattern 'const MarkdownPreview = $$$'
# Also check for any markdown processing or rendering logic
rg -A 5 "markdown|dangerouslySetInnerHTML" --type typescript --type javascriptreact --type typescriptreact
Length of output: 8788
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DoctorNote.tsx (2)
34-34: Consider making height constraint more responsive.The fixed height of
500pxmight not be optimal for all screen sizes. Consider using:
- CSS clamp for dynamic height
- vh units for viewport-relative sizing
- CSS custom properties for configurable heights
- className="mt-4 flex h-[500px] grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2" + className="mt-4 flex h-clamp(400px, 70vh, 800px) grow flex-col-reverse overflow-y-scroll bg-white sm:ml-2"Also applies to: 46-46
51-80: Add defensive checks for mode and note properties.While the logic is well-structured, consider adding defensive checks:
- Validate note.id existence before using as key
- Handle undefined mode more explicitly
- Add type guard for note.root_note_object
{state.notes.map((note) => { + if (!note?.id) { + console.warn('Note missing ID:', note); + return null; + } const noteCard = ( <PatientNoteCard note={note} setReload={setReload} disableEdit={disableEdit} setReplyTo={setReplyTo} - mode={mode} + mode={mode ?? 'default-view'} allowThreadView={mode === "thread-view"} allowReply={mode !== "thread-view"} setThreadViewNote={setThreadViewNote} /> ); - if (mode === "thread-view" && !note.root_note_object) { + const isThreadView = mode === "thread-view"; + const hasNoRootNote = !note.root_note_object; + if (isThreadView && hasNoRootNote) {🧰 Tools
🪛 Biome
[error] 53-62: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
src/components/Common/MarkdownPreview.tsx (1)
22-38: Enhance type safety for mentioned users handlingThe current implementation could benefit from stronger typing:
- Add runtime validation for mentioned users
- Use a more specific type for the users map
Consider this improvement:
+type MentionedUsersMap = Record<string, UserBareMinimum>; + const MarkdownPreview = ({ markdown, mentioned_users, }: { markdown: string; mentioned_users?: UserBareMinimum[]; }) => { - const MentionedUsers = Object.fromEntries( + const MentionedUsers: MentionedUsersMap = Object.fromEntries( mentioned_users?.map((u) => [u.username, u]) ?? [], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Common/MarkdownPreview.tsx(1 hunks)src/components/Facility/DoctorNote.tsx(2 hunks)
🧰 Additional context used
🪛 Biome
src/components/Common/MarkdownPreview.tsx
[error] 132-132: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
src/components/Facility/DoctorNote.tsx
[error] 53-62: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (7)
src/components/Facility/DoctorNote.tsx (2)
17-18: LGTM! Well-defined type constraints for the mode prop.
The union type "thread-view" | "default-view" ensures type safety and makes the allowed values explicit.
22-30: LGTM! Clean props destructuring.
The destructuring is well-organized and matches the interface definition.
src/components/Common/MarkdownPreview.tsx (5)
39-61: LGTM! Secure link handling implementation
The link renderer implementation is robust with:
- Proper URL validation
- Security attributes (noopener, noreferrer)
- Graceful error handling
87-128: LGTM! Clean event handling implementation
The event handling implementation is well-structured with:
- Proper event listener cleanup
- Efficient event delegation
- Accurate position calculation
7-20: 🛠️ Refactor suggestion
Add error handling and accessibility improvements to UserCard
The component needs defensive programming and accessibility enhancements:
- Missing error handling for undefined user properties
- Missing accessibility attributes for better screen reader support
Apply this diff:
const UserCard = ({ user }: { user: UserBareMinimum }) => (
<div className="z-10 flex w-64 items-center space-x-3 rounded-lg bg-gray-200 px-3 pb-3 shadow-lg">
- <div className="flex h-10 w-10 items-center justify-center rounded-full bg-primary text-lg font-semibold text-white">
- {user.first_name[0]}
+ <div
+ className="flex h-10 w-10 items-center justify-center rounded-full bg-primary text-lg font-semibold text-white"
+ role="img"
+ aria-label={`${user.first_name} ${user.last_name}'s avatar`}
+ >
+ {user.first_name?.[0] ?? '?'}
</div>
<div className="space-y-0">
- <h3 className="text-sm font-semibold text-gray-800">
- {user.first_name} {user.last_name}
+ <h3 className="text-sm font-semibold text-gray-800" title={`${user.first_name} ${user.last_name}`}>
+ {[user.first_name, user.last_name].filter(Boolean).join(' ') || 'Unknown User'}
</h3>
- <p className="text-xs text-gray-500">@{user.username}</p>
- <p className="text-xs text-gray-500">{user.user_type}</p>
+ <p className="text-xs text-gray-500">@{user.username || 'unknown'}</p>
+ <p className="text-xs text-gray-500">{user.user_type || 'User'}</p>
</div>
</div>
);Likely invalid or redundant comment.
83-85: 🛠️ Refactor suggestion
Strengthen HTML sanitization configuration
While DOMPurify is correctly used, the configuration could be more restrictive for better security.
Apply this improvement:
- const sanitizedHtml = DOMPurify.sanitize(html, {
- ADD_ATTR: ["target", "rel"],
- });
+ const sanitizedHtml = DOMPurify.sanitize(html, {
+ ALLOWED_TAGS: ['p', 'a', 'b', 'i', 'em', 'strong', 'del', 'span'],
+ ALLOWED_ATTR: ['href', 'target', 'rel', 'class', 'data-username'],
+ ALLOWED_URI_REGEXP: /^(?:(?:(?:f|ht)tps?|mailto|tel|callto|cid|xmpp):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i,
+ ADD_ATTR: ['target', 'rel'],
+ });Likely invalid or redundant comment.
134-143: 🛠️ Refactor suggestion
Improve hover card positioning for viewport boundaries
The current fixed positioning can cause issues when the hover card appears near viewport edges.
Apply this improvement:
<div
- className="fixed z-50 transition-opacity duration-300 ease-in-out"
+ className="absolute z-50 transition-opacity duration-300 ease-in-out"
style={{
- top: `${hoverPosition.y - 100}px`,
- left: `${hoverPosition.x}px`,
+ top: '0',
+ left: '0',
+ transform: `translate(
+ max(
+ ${hoverPosition.x}px,
+ ${Math.min(hoverPosition.x, window.innerWidth - 320)}px
+ ),
+ max(
+ ${hoverPosition.y - 100}px,
+ 10px
+ )
+ )`,
}}
>Likely invalid or redundant comment.
|
This PR is created to track my progress on the GSoC project (#7159) and to merge all PRs related to Doctor notes.
Backend RR: ohcnetwork/care#2295
walk_through_discussion_notes_enhancement.mp4
File Upload Option
User Tagging Mechanism
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores