Enhancements to Encounter Discussion/Notes | Fix Translation #10156
Conversation
WalkthroughThis pull request enhances the encounter discussion UI by updating user navigation and thread management logic. In the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rajku-dev
changed the title
rajku-dev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (1)
Line range hint
1-1: Implement remaining PR objectives.The following objectives from the PR description are not yet implemented:
- Adding unique identifier labeling for threads
- Displaying the total number of participants in a thread
Would you like me to help with implementing these features?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Encounters/tabs/EncounterNotesTab.tsx(7 hunks)src/types/notes/messages.ts(1 hunks)
🔇 Additional comments (5)
src/types/notes/messages.ts (1)
8-8: LGTM! The new property supports relative time display.The addition of
created_dateproperty aligns with the PR objective of displaying relative time for messages.Let's verify the impact of this breaking change:
✅ Verification successful
Verified: The created_date property is properly integrated
The addition of
created_dateproperty to the Message interface is safe and well-supported:
- Used in encounter notes for displaying relative time
- All implementations handle the property correctly through API endpoints
- No direct Message object creations found that would break with this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Message interface usage to ensure all implementations handle the new required property ast-grep --pattern 'interface $_: Message { $$$ }'Length of output: 54
Script:
#!/bin/bash # First, find files containing "Message" type/interface echo "=== Files containing Message type/interface ===" rg "Message" --type typescript echo -e "\n=== Direct Message interface implementations ===" ast-grep --pattern 'type $_ = Message' echo -e "\n=== Message interface usage in type declarations ===" ast-grep --pattern 'const $_: Message = {' echo -e "\n=== Message interface extensions ===" ast-grep --pattern 'interface $_ extends Message'Length of output: 631
Script:
#!/bin/bash # Search in TypeScript files echo "=== Files containing Message ===" rg "Message" -g "*.ts" -g "*.tsx" echo -e "\n=== Message interface definition and usage ===" ast-grep --pattern 'interface Message { $$$ }' echo -e "\n=== Message type usage ===" rg "Message" -g "*.ts" -g "*.tsx" -A 2 -B 2Length of output: 72935
src/pages/Encounters/tabs/EncounterNotesTab.tsx (4)
16-16: LGTM! New imports support the PR objectives.The added imports enable user profile navigation and relative time display features.
Also applies to: 56-56
123-123: LGTM! User profile navigation implemented correctly.The implementation properly links user avatars to their profile pages using the facility context.
Also applies to: 141-153
171-171: LGTM! Message styling and relative time display look good.The changes improve message layout and properly implement the relative time display.
Also applies to: 178-182
253-253: Verify text casing consistency across the application.The button text has been changed to lowercase. Let's verify if this aligns with the application's text casing guidelines.
Also applies to: 264-264
✅ Verification successful
Button text casing change aligns with application pattern
The change to lowercase for "cancel" and "create" button text is consistent with the established pattern across the codebase, where button text translations consistently use lowercase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for button text casing patterns in the codebase rg -i "t\([\"'](?:cancel|create)[\"']\)" --type ts --type tsxLength of output: 95
Script:
#!/bin/bash # Search for translation patterns in TypeScript files rg "t\(['\"].*?['\"]" --type ts # Also search in the current directory for any .tsx files find . -name "*.tsx" -type f -exec grep -H "t(['\"]" {} \;Length of output: 80276
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (2)
551-554: Consider memoizing thread statistics.While the implementation is correct, computing participant count on every render could be optimized:
+ const participantCount = useMemo( + () => new Set(messages.map((m) => m.created_by.id)).size, + [messages] + ); <span> - {new Set(messages.map((m) => m.created_by.id)).size} + {participantCount} </span>
Line range hint
1-683: Well-structured component with good separation of concerns.The component demonstrates good architectural practices:
- Clear separation between mobile and desktop views
- Proper state management using React Query
- Modular component breakdown
- Consistent error handling with toast notifications
Consider splitting the large component into smaller ones for better maintainability:
- Move ThreadList to a separate component
- Extract MessageList as a standalone component
- Create a separate MessageInput component
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (4)
11-11: LGTM! New imports align with feature requirements.The added imports support the new features:
MessageSquarefor message count displayLinkandusePathParamsfor profile navigationrelativeTimefor timestamp formattingAlso applies to: 17-17, 57-57
124-124: Profile navigation implementation looks good!The implementation correctly:
- Extracts facilityId for constructing profile URLs
- Wraps the avatar in a Link component for navigation
- Maintains the tooltip functionality
Also applies to: 142-154
172-172: Message styling and timestamp display enhanced.Good improvements:
- Better padding and margin for improved readability
- Relative time display added using the created_date property
- Markdown content properly structured with timestamp
Also applies to: 179-183
254-254: Verify text casing convention.The button text has been changed to lowercase. Please ensure this aligns with the design system's text casing guidelines for buttons.
Also applies to: 265-265
✅ Verification successful
Translation key casing is correct ✓
The lowercase translation keys
t("cancel")andt("create")follow the established convention used consistently throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for button text casing patterns in the codebase rg -i "t\([\"'](?:cancel|create)[\"']\)" --type ts --type tsxLength of output: 95
Script:
#!/bin/bash # Search for translation patterns for cancel/create buttons with context rg -A 2 -B 2 "t\(['\"](?i)(cancel|create)['\"]" # Also search for direct button text patterns to understand casing conventions rg -A 2 -B 2 "<Button.*>.*(?i)(cancel|create).*</Button>"Length of output: 13340
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (2)
124-124: Consider extracting the profile URL construction.The profile URL construction could be made more maintainable by extracting it to a utility function or constant.
+const getUserProfileUrl = (facilityId: string, username: string) => + `/facility/${facilityId}/users/${username}`; const MessageItem = ({ message }: { message: Message }) => { const authUser = useAuthUser(); const { facilityId } = usePathParams("/facility/:facilityId/*")!; // ... - <Link href={`/facility/${facilityId}/users/${message.created_by.username}`}> + <Link href={getUserProfileUrl(facilityId, message.created_by.username)}>Also applies to: 142-154
558-561: Consider memoizing the participant count calculation.The participant count calculation using
Setis performed on every render. Consider memoizing it for better performance.+const useParticipantCount = (messages: Message[]) => + useMemo( + () => new Set(messages.map((m) => m.created_by.id)).size, + [messages] + ); // In the component: +const participantCount = useParticipantCount(messages); <div className="flex items-center gap-2 text-xs text-gray-500"> <Users className="h-4 w-4" /> - <span>{new Set(messages.map((m) => m.created_by.id)).size}</span> + <span>{participantCount}</span> <MessageSquare className="h-4 w-4 ml-3" /> <span>{messages.length}</span> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx(8 hunks)
🔇 Additional comments (3)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (3)
11-11: LGTM! New imports enhance the functionality.The added imports support the new features:
- MessageSquare icon for message count display
- Link and usePathParams for profile navigation
- relativeTime for relative timestamp display
Also applies to: 17-17, 57-57
179-190: Great implementation of the timestamp display!The timestamp implementation provides excellent user experience by:
- Showing relative time for quick reference
- Providing exact timestamp in the tooltip
- Using proper date formatting
261-261: LGTM! Consistent button text casing.The button text changes maintain consistency with the application's style guide.
Also applies to: 272-272
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (3)
142-154: Enhance accessibility for the avatar link.While the implementation looks good, consider adding an aria-label to improve accessibility for screen readers.
<Link href={`/facility/${facilityId}/users/${message.created_by.username}`} + aria-label={`View ${message.created_by.username}'s profile`} >
179-190: Improve timestamp tooltip accessibility.The timestamp implementation with relative and absolute time is good, but could be more accessible.
-<TooltipTrigger> +<TooltipTrigger asChild> + <button + type="button" + className="text-xs text-gray-500" + aria-label={`Message sent ${new Date(message.created_date).toLocaleString()}`} + > {relativeTime(message.created_date)} + </button> </TooltipTrigger>
562-580: Optimize participant count and add translations.Consider memoizing the participant count and adding translations for the tooltip content.
+const participantCount = useMemo( + () => new Set(messages.map((m) => m.created_by.id)).size, + [messages] +); <TooltipContent> <p> - Participants:{" "} - {new Set(messages.map((m) => m.created_by.id)).size} + {t("encounter_notes__participants_count", { count: participantCount })} </p> <p> - Messages: {messages.length} + {t("encounter_notes__messages_count", { count: messages.length })} </p> </TooltipContent>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (1)
11-11: LGTM! New imports support the enhanced features.The added imports align with the PR objectives for implementing relative time display, user profile navigation, and message statistics.
Also applies to: 17-17, 57-57
|
@Jacobjeevan Please review the changes. Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (3)
66-74: Consider using a different approach for thread template filtering.The thread templates array is made mutable to filter out existing thread titles. Consider maintaining immutability by creating a new filtered array instead.
-let threadTemplates = [ +const THREAD_TEMPLATES = [ "Treatment Plan", "Medication Notes", "Care Coordination", "General Notes", "Patient History", "Referral Notes", "Lab Results Discussion", ];Then filter when needed:
const availableTemplates = THREAD_TEMPLATES.filter( template => !threadsData?.results.some(thread => thread.title === template) );
336-341: Optimize thread template filtering.The template filtering in useEffect depends on both threadsData and selectedThread, but selectedThread isn't used in the filtering logic. This could cause unnecessary re-filtering.
- useEffect(() => { + useEffect(() => { if (threadsData?.results.length) { if (!selectedThread) setSelectedThread(threadsData.results[0].id); + } + }, [threadsData, selectedThread]); + + useEffect(() => { + if (threadsData?.results.length) { const threadTitles = threadsData.results.map((thread) => thread.title); threadTemplates = threadTemplates.filter( (template) => !threadTitles.includes(template), ); } - }, [threadsData, selectedThread]); + }, [threadsData]);
568-586: Memoize participant and message counts.The counts are recalculated on every render. Consider memoizing them to improve performance.
+const useChatStats = (messages: Message[]) => { + return useMemo(() => ({ + participantCount: new Set(messages.map((m) => m.created_by.id)).size, + messageCount: messages.length + }), [messages]); +}; +const { participantCount, messageCount } = useChatStats(messages); <TooltipTrigger asChild> <div className="flex items-center gap-2 text-xs text-gray-500 cursor-pointer"> <Users className="h-4 w-4" /> - <span> - {new Set(messages.map((m) => m.created_by.id)).size} - </span> + <span>{participantCount}</span> <MessageSquare className="h-4 w-4 ml-3" /> - <span>{messages.length}</span> + <span>{messageCount}</span> </div> </TooltipTrigger>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/pages/Encounters/tabs/EncounterNotesTab.tsx(12 hunks)
🔇 Additional comments (4)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (4)
143-155: Well-implemented user profile navigation!The avatar now serves as a clickable link to the user's profile, improving navigation while maintaining the tooltip functionality.
180-191: Good implementation of relative timestamps!The message timestamp implementation is well done with:
- Relative time for quick reference
- Full timestamp on hover
- Proper alignment and styling
427-432: Strengthen thread title validation.The duplicate check should be case-insensitive to prevent similar titles with different casing.
if ( - threadsData?.results.some((thread) => thread.title === title.trim()) + threadsData?.results.some( + (thread) => thread.title.toLowerCase() === title.trim().toLowerCase() + ) ) { - toast.error(t("thread_already_exists")); + toast.error(t("encounter_notes__duplicate_thread_title")); return; }
407-407: Good addition of error message translation!The error message is now properly internationalized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (1)
427-432:⚠️ Potential issueStrengthen thread title validation.
The thread title validation should be case-insensitive to prevent similar titles with different casing.
if ( - threadsData?.results.some((thread) => thread.title === title.trim()) + threadsData?.results.some( + (thread) => thread.title.toLowerCase() === title.trim().toLowerCase() + ) ) { toast.error(t("thread_already_exists")); return; }
🧹 Nitpick comments (3)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (3)
143-155: Add accessibility attributes to the avatar link.While the avatar link implementation is good, it needs accessibility improvements.
<Link href={`/facility/${facilityId}/users/${message.created_by.username}`} + aria-label={`View ${message.created_by.username}'s profile`} > <TooltipTrigger asChild> - <div className="flex pr-2"> + <div className="flex pr-2" role="img" aria-label={`${message.created_by.username}'s avatar`}> <Avatar name={message.created_by.username} imageUrl={message.created_by.profile_picture_url} className="w-8 h-8 rounded-full object-cover ring-1 ring-transparent hover:ring-red-200 transition" /> </div> </TooltipTrigger> </Link>
180-191: Consider using Intl.DateTimeFormat for consistent date formatting.The timestamp implementation is good, but using Intl.DateTimeFormat would provide better localization support.
<div className="mt-2"> <Markdown content={message.message} className="text-sm mb-2" /> <span className="text-xs text-gray-500 flex justify-end"> <Tooltip> <TooltipTrigger> {relativeTime(message.created_date)} </TooltipTrigger> <TooltipContent> - {new Date(message.created_date).toLocaleString()} + {new Intl.DateTimeFormat(undefined, { + dateStyle: 'full', + timeStyle: 'long' + }).format(new Date(message.created_date))} </TooltipContent> </Tooltip> </span> </div>
568-586: Optimize participant counting logic.The current implementation recalculates participants on every render. Consider memoizing this calculation.
+const useParticipantCount = (messages: Message[]) => { + return useMemo(() => + new Set(messages.map((m) => m.created_by.id)).size, + [messages] + ); +}; +const participantCount = useParticipantCount(messages); <Tooltip> <TooltipTrigger asChild> <div className="flex items-center gap-2 text-xs text-gray-500 cursor-pointer"> <Users className="h-4 w-4" /> - <span> - {new Set(messages.map((m) => m.created_by.id)).size} - </span> + <span>{participantCount}</span> <MessageSquare className="h-4 w-4 ml-3" /> <span>{messages.length}</span> </div> </TooltipTrigger> <TooltipContent> <p> - Participants:{" "} - {new Set(messages.map((m) => m.created_by.id)).size} + Participants: {participantCount} </p> <p>Messages: {messages.length}</p> </TooltipContent> </Tooltip>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/pages/Encounters/tabs/EncounterNotesTab.tsx(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (1)
11-11: LGTM! New imports are appropriate.The added imports support the new features: MessageSquare for icons, Link and usePathParams for navigation, and relativeTime for timestamp formatting.
Also applies to: 17-17, 58-58
public/locale/en.json (1)
965-965: Maintain consistent translation key naming convention.The translation keys follow different naming conventions:
failed_to_send_messageuses snake_casethread_already_existsuses snake_caseConsider prefixing both with the feature namespace:
- "failed_to_send_message": "Failed to send message", + "encounter_notes__failed_send_message": "Failed to send message", ... - "thread_already_exists": "Thread with this title already exists", + "encounter_notes__duplicate_thread_title": "Thread with this title already exists"Also applies to: 1972-1972
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (3)
174-174: Consider using design tokens for colors.Replace hardcoded color values with design system tokens for better maintainability and consistency.
-"pl-2 pr-1 pt-1 rounded-lg break-words border border-primary-200 relative", +"pl-2 pr-1 pt-1 rounded-lg break-words border border-[--primary-200] relative",
405-417: Optimize thread management implementation.Consider these improvements for better performance and readability:
- Use
useMemofor threads state initialization- Simplify the filter operation
-const [threads, setThreads] = useState<string[]>([...threadTemplates]); +const [threads, setThreads] = useState<string[]>(() => [...threadTemplates]); useEffect(() => { if (threadsData?.results.length) { if (!selectedThread) setSelectedThread(threadsData.results[0].id); - const threadTitles = threadsData.results.map((thread) => thread.title); - setThreads( - threads.filter((template) => !threadTitles.includes(template)), - ); + setThreads((currentThreads) => + currentThreads.filter( + (template) => !threadsData.results.some((thread) => thread.title === template) + ) + ); } }, [threadsData, selectedThread]);
576-594: Optimize thread statistics implementation.Consider these improvements:
- Memoize participant count calculation
- Extract statistics display into a reusable component
+const ThreadStats = ({ messages }: { messages: Message[] }) => { + const participantCount = useMemo( + () => new Set(messages.map((m) => m.created_by.id)).size, + [messages] + ); + + return ( + <Tooltip> + <TooltipTrigger asChild> + <div className="flex items-center gap-2 text-xs text-gray-500 cursor-pointer"> + <Users className="h-4 w-4" /> + <span>{participantCount}</span> + <MessageSquare className="h-4 w-4 ml-3" /> + <span>{messages.length}</span> + </div> + </TooltipTrigger> + <TooltipContent> + <p>Participants: {participantCount}</p> + <p>Messages: {messages.length}</p> + </TooltipContent> + </Tooltip> + ); +}; // Replace existing implementation with: <ThreadStats messages={messages} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx(14 hunks)
🔇 Additional comments (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (1)
435-440: 🛠️ Refactor suggestionStrengthen thread title validation.
The duplicate check should be case-insensitive to prevent similar titles with different casing.
if ( - threadsData?.results.some((thread) => thread.title === title.trim()) + threadsData?.results.some( + (thread) => thread.title.toLowerCase() === title.trim().toLowerCase() + ) ) { - toast.error(t("thread_already_exists")); + toast.error(t("encounter_notes__duplicate_thread_title")); return; }Likely invalid or redundant comment.
Sure, I have tried solving it, can you check it once more @nihal467 |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (2)
348-359: Fix potential memory leak in AbortController usage.The current implementation creates a new AbortController but never aborts it. This could lead to memory leaks.
- const response = await query(routes.notes.patient.getMessages, { + const controller = new AbortController(); + try { + const response = await query(routes.notes.patient.getMessages, { + pathParams: { + patientId: encounter.patient.id, + threadId: selectedThread!, + }, + queryParams: { + limit: String(MESSAGES_LIMIT), + offset: String(pageParam), + }, + })({ signal: controller.signal }); + return response as PaginatedResponse<Message>; + } finally { + controller.abort(); + } - pathParams: { - patientId: encounter.patient.id, - threadId: selectedThread!, - }, - queryParams: { - limit: String(MESSAGES_LIMIT), - offset: String(pageParam), - }, - })({ signal: new AbortController().signal }); - return response as PaginatedResponse<Message>;
448-461: Strengthen thread title validation.The thread title validation should be case-insensitive to prevent similar titles with different casing.
- if ( - threadsData?.results.some((thread) => thread.title === title.trim()) - ) { + const normalizedTitle = title.trim().toLowerCase(); + if ( + threadsData?.results.some( + (thread) => thread.title.toLowerCase() === normalizedTitle + ) + ) { toast.error(t("thread_already_exists")); return; }
🧹 Nitpick comments (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (1)
142-161: Add aria-label to improve accessibility.The avatar link should have an aria-label to improve accessibility for screen readers.
<Link href={`/facility/${facilityId}/users/${message.created_by.username}`} + aria-label={`View ${message.created_by.username}'s profile`} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (3)
11-11: LGTM! New imports support the added features.The new imports for MessageSquare, Link, usePathParams, and relativeTime are appropriately added to support the new functionality.
Also applies to: 17-17, 58-58
180-193: LGTM! Good use of tooltips for timestamp details.The implementation nicely combines relative time for quick reference with a detailed timestamp in the tooltip.
591-609: LGTM! Good use of tooltips for thread statistics.The implementation provides a clean UI with detailed information available through tooltips.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (1)
456-469: Strengthen thread title validation.The current validation doesn't handle case sensitivity and doesn't trim whitespace consistently.
const handleCreateThread = (title: string) => { - if (title.trim()) { + const normalizedTitle = title.trim(); + if (normalizedTitle) { if ( - threadsData?.results.some((thread) => thread.title === title.trim()) + threadsData?.results.some( + (thread) => thread.title.toLowerCase() === normalizedTitle.toLowerCase() + ) ) { toast.error(t("thread_already_exists")); return; } createThreadMutation.mutate({ - title: title.trim(), + title: normalizedTitle, encounter: encounter.id, }); } };
🧹 Nitpick comments (4)
src/pages/Encounters/tabs/EncounterNotesTab.tsx (4)
141-160: Consider simplifying nested tooltips for better accessibility.The current implementation nests tooltips (outer tooltip wraps the Link+Avatar, inner tooltip shows username), which might cause accessibility and usability issues. Consider simplifying by keeping only one tooltip or using a more accessible pattern.
- <TooltipProvider> - <Tooltip> - <Link - href={`/facility/${facilityId}/users/${message.created_by.username}`} - > - <TooltipTrigger asChild> - <div className="flex pr-2"> - <Avatar - name={message.created_by.username} - imageUrl={message.created_by.profile_picture_url} - className="w-8 h-8 rounded-full object-cover ring-1 ring-transparent hover:ring-red-200 transition" - /> - </div> - </TooltipTrigger> - </Link> - <TooltipContent> - <p>{message.created_by.username}</p> - </TooltipContent> - </Tooltip> - </TooltipProvider> + <Link + href={`/facility/${facilityId}/users/${message.created_by.username}`} + className="group relative" + > + <Avatar + name={message.created_by.username} + imageUrl={message.created_by.profile_picture_url} + className="w-8 h-8 rounded-full object-cover ring-1 ring-transparent hover:ring-red-200 transition" + /> + <span className="absolute -bottom-1 left-1/2 transform -translate-x-1/2 bg-gray-900 text-white text-xs px-2 py-1 rounded opacity-0 group-hover:opacity-100 transition-opacity"> + {message.created_by.username} + </span> + </Link>
250-262: Optimize template filtering performance.The template filtering could be memoized to prevent unnecessary re-renders.
+const useMemoizedThreads = (templates: string[], existingThreads: Thread[]) => { + return useMemo( + () => templates.filter( + (template) => !existingThreads.some( + (thread) => thread.title.toLowerCase() === template.toLowerCase() + ) + ), + [templates, existingThreads] + ); +}; // In the component: -const [threads, setThreads] = useState<string[]>([...threadTemplates]); +const threads = useMemoizedThreads(threadTemplates, threadsData?.results ?? []);
444-454: Optimize scroll behavior for better performance.The current scroll implementation might cause unnecessary re-renders and janky scrolling behavior.
+const useScrollToBottom = ( + ref: RefObject<HTMLDivElement>, + deps: any[], + condition: boolean = true +) => { + useEffect(() => { + if (condition && ref.current) { + ref.current.scrollIntoView({ behavior: 'smooth' }); + } + }, deps); +}; -useEffect(() => { - if ( - messagesData && - !messagesLoading && - !isFetchingNextPage && - nextPageFetching - ) { - messagesEndRef.current?.scrollIntoView(); - setNextPageFetching(false); - } -}, [selectedThread, messagesData, messagesLoading, isFetchingNextPage]); +useScrollToBottom( + messagesEndRef, + [selectedThread, messagesData?.pages.length], + !isFetchingNextPage && nextPageFetching +);
599-617: Optimize participant counting logic.The current implementation recalculates participants on every render. Consider memoizing this calculation.
+const useThreadStats = (messages: Message[]) => { + return useMemo(() => ({ + participantCount: new Set(messages.map(m => m.created_by.id)).size, + messageCount: messages.length + }), [messages]); +}; +const stats = useThreadStats(messages); -<span>{new Set(messages.map((m) => m.created_by.id)).size}</span> +<span>{stats.participantCount}</span> -<span>{messages.length}</span> +<span>{stats.messageCount}</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Encounters/tabs/EncounterNotesTab.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
rithviknishad
added
invalid
Co-authored-by: Rithvik Nishad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
public/locale/en.json (1)
863-863: Minor Grammatical Refinement in Translation MessageThe translation string in line 863 is clear; however, consider adding an article for improved readability. For example, update the string to:
"encounter_notes__no_unused_threads": "Please enter a custom title for the thread"
This minor change can enhance grammatical correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(5 hunks)src/pages/Encounters/tabs/EncounterNotesTab.tsx(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Encounters/tabs/EncounterNotesTab.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
public/locale/en.json (1)
2027-2027: Ensure Consistent Translation Key NamingThe key
"thread_already_exists"does not follow the namespaced naming pattern used in other encounterNotes-related keys (such as"encounter_notes__no_unused_threads"). To ensure consistency across the locale file, consider renaming this key to something like"encounter_notes__duplicate_thread_title". This approach is in line with past recommendations for translation key naming.Suggested diff:
- "thread_already_exists": "Thread with this title already exists", + "encounter_notes__duplicate_thread_title": "Thread with this title already exists",
Proposed Changes
Recording.2025-01-26.052528.mp4
Recording.2025-01-26.061335.mp4
INFINITE SCROLL
Uploading Recording 2025-02-04 144900.mp4…
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit