Adds useInfiniteQuery data fetching hook + fixes Inifinite Load issue in notes
#9190
Conversation
WalkthroughThe changes primarily involve updating several components related to patient notes management by replacing manual data fetching with the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PatientNotesList
participant useInfiniteQuery
participant DoctorNote
User->>PatientNotesList: Add Note
PatientNotesList->>useInfiniteQuery: Fetch Notes
useInfiniteQuery-->>PatientNotesList: Return Notes
PatientNotesList->>User: Display Updated Notes
User->>DoctorNote: Scroll Down
DoctorNote->>useInfiniteQuery: Check for More Notes
useInfiniteQuery-->>DoctorNote: Return hasMore
DoctorNote->>User: Load More Notes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/PatientNotesList.tsx (1)
Line range hint
1-105: Consider implementing a more robust infinite scroll solutionThe current implementation might benefit from using established infinite scroll libraries or implementing virtual scrolling to handle large datasets more efficiently.
Consider these architectural improvements:
- Use a virtual scroll library like
react-windoworreact-virtualizedto efficiently render large lists- Implement scroll position restoration using
ScrollRestorationfromreact-router-dom- Add loading states for individual batches to show progress without full-screen loading
- Consider implementing a cursor-based pagination instead of offset-based to prevent issues with concurrent updates
Example implementation structure:
import { FixedSizeList } from 'react-window'; interface NotesListProps { // ... existing props windowHeight: number; } const NotesListRow = ({ index, style, data }) => { const note = data.notes[index]; return ( <div style={style}> <DoctorNote note={note} /> </div> ); }; const PatientNotesList = (props: NotesListProps) => { const [scrollOffset, setScrollOffset] = useState(0); // ... existing state and effects return ( <FixedSizeList height={props.windowHeight} itemCount={state.notes.length} itemSize={120} onScroll={({ scrollOffset }) => { setScrollOffset(scrollOffset); // Trigger load more when near bottom }} > {NotesListRow} </FixedSizeList> ); };src/components/Facility/PatientConsultationNotesList.tsx (1)
Line range hint
41-77: Enhance infinite scroll implementation to prevent unexpected reloadsThe current implementation might be causing the reported issues with unexpected reloads and lost scroll position. Several improvements are recommended:
- The scroll position issue mentioned in Enhance Discussion Notes Chat History with Infinite Scrolling or Better Navigation Solution #9188 likely occurs because state updates trigger re-renders without preserving scroll position.
- Multiple rapid scroll events could trigger unnecessary fetches.
- The loading state might cause layout shifts during updates.
Consider these improvements:
- Implement scroll position preservation:
const scrollRef = useRef<HTMLDivElement>(null); const scrollPosition = useRef(0); const saveScrollPosition = () => { if (scrollRef.current) { scrollPosition.current = scrollRef.current.scrollTop; } }; const restoreScrollPosition = () => { if (scrollRef.current) { scrollRef.current.scrollTop = scrollPosition.current; } }; // Save position before fetch saveScrollPosition(); fetchNotes().then(() => { setIsLoading(false); setReload?.(false); // Restore position after state update requestAnimationFrame(restoreScrollPosition); });
- Add debouncing to prevent rapid fetches:
const debouncedHandleNext = useMemo( () => debounce(() => { if (state.cPage < state.totalPages) { setState((prevState) => ({ ...prevState, cPage: prevState.cPage + 1, })); setReload?.(true); } }, 250), [state.cPage, state.totalPages] );
- Consider using a virtual scroll library like
react-windowfor better performance with large lists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Facility/PatientConsultationNotesList.tsx(1 hunks)src/components/Facility/PatientNotesList.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Facility/PatientNotesList.tsx (1)
67-68: 🛠️ Refactor suggestion
Verify scroll position preservation during thread changes
Setting both loading and reload states simultaneously on thread change might cause scroll position loss, which was mentioned as a key issue in the original bug report.
Let's verify the scroll handling in related components:
Consider debouncing the reload trigger and preserving scroll position:
useEffect(() => {
+ const currentScrollPosition = window.scrollY;
setIsLoading(true);
- setReload(true);
+ // Debounce reload to prevent rapid re-fetches
+ const timeoutId = setTimeout(() => {
+ setReload(true);
+ }, 100);
+ // Restore scroll position after state updates
+ requestAnimationFrame(() => {
+ window.scrollTo(0, currentScrollPosition);
+ });
+ return () => clearTimeout(timeoutId);
}, [thread]);src/components/Facility/PatientConsultationNotesList.tsx (1)
Line range hint 1-1: Verify similar scroll handling in related components
Let's check if similar infinite scroll patterns exist in other components that might need the same improvements.
|
LGTM |
…issues/9188/discussion-note-infinite-scroll-bug
…issues/9188/discussion-note-infinite-scroll-bug
|
@JavidSumra what is the status on this PR |
|
Hey @nihal467 I'm working on |
…issues/9188/discussion-note-infinite-scroll-bug
…ps://github.com/JavidSumra/care_fe into issues/9188/discussion-note-infinite-scroll-bug
…ps://github.com/JavidSumra/care_fe into issues/9188/discussion-note-infinite-scroll-bug
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/PatientNotesList.tsx (2)
43-48: Consider memoizing the setState callbackTo prevent unnecessary rerenders, consider memoizing the setState callback using useCallback:
+ const updateNotes = useCallback((prevState: PatientNoteStateType) => ({ + ...prevState, + notes, + }), [notes]); useEffect(() => { - setState((prevState: any) => ({ - ...prevState, - notes, - })); + setState(updateNotes); }, [notes, setState, updateNotes]);
Line range hint
56-71: Improve loading state handlingThe current loading condition might not cover all edge cases. Consider showing a loading indicator during subsequent page loads as well.
- if ((loading && reload) || !state.notes.length) { + if (loading && (!state.notes.length || reload)) { return ( <div className="flex h-full w-full items-center justify-center bg-white"> <CircularProgress /> </div> ); } + // Show a bottom loader during pagination + return ( + <> <DoctorNote state={state} handleNext={fetchNextPage} setReload={refetch} setReplyTo={setReplyTo} hasMore={hasMore} /> + {loading && state.notes.length > 0 && ( + <div className="flex justify-center p-4"> + <CircularProgress size="small" /> + </div> + )} + </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/components/Facility/ConsultationDoctorNotes/index.tsx(1 hunks)src/components/Facility/PatientConsultationNotesList.tsx(4 hunks)src/components/Facility/PatientNotesList.tsx(4 hunks)src/components/Facility/PatientNotesSlideover.tsx(1 hunks)src/components/Patient/PatientDetailsTab/Notes.tsx(1 hunks)src/components/Patient/PatientNotes.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/Facility/PatientNotesSlideover.tsx
- src/components/Facility/ConsultationDoctorNotes/index.tsx
- src/components/Facility/PatientConsultationNotesList.tsx
- src/components/Patient/PatientNotes.tsx
- src/components/Patient/PatientDetailsTab/Notes.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Facility/PatientNotesList.tsx (1)
Learnt from: UdaySagar-Git
PR: ohcnetwork/care_fe#9298
File: src/components/Facility/PatientNotesDetailedView.tsx:147-188
Timestamp: 2024-12-05T22:41:24.173Z
Learning: In the `PatientNotesDetailedView` component, child notes (`state.child_notes`) are fetched along with the parent note in a single query, so a separate loading state for child notes is not required.
🔇 Additional comments (2)
src/components/Facility/PatientNotesList.tsx (2)
Line range hint 1-11: LGTM! Clean integration of useInfiniteQuery
The import structure is well-organized and the addition of useInfiniteQuery aligns with the PR objectives.
26-41: Verify scroll position maintenance during infinite loading
The implementation looks good for fixing the infinite scroll issues. Let's verify that the scroll position is maintained during data fetching.
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the scroll position maintenance implementation:
Scroll position is properly maintained during infinite loading
The implementation uses useInfiniteQuery hook which handles pagination while preserving the existing items in the list. The code shows:
- Proper deduplication of items using unique IDs
- Maintains existing items while fetching next page
- No forced reloads that would reset scroll position
- Efficient offset-based pagination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for scroll position management in the codebase
rg -A 5 "scroll" --type ts --type tsx
# Look for any scroll event listeners
ast-grep --pattern 'addEventListener("scroll", $_)'
Length of output: 119
Script:
#!/bin/bash
# Search for scroll position management in the codebase with correct file extensions
rg -A 5 "scroll" -t typescript -t tsx
# Look for scroll-related hooks or components in the codebase
rg -A 5 "useInfiniteScroll|InfiniteScroll" -t typescript -t tsx
# Look for any scroll event listeners with different pattern
ast-grep --pattern 'onScroll={$_}'
Length of output: 210
Script:
#!/bin/bash
# Search for files first
fd -e ts -e tsx
# Then search for scroll-related content
rg "scroll" --type-add 'ts:*.{ts,tsx}' -t ts
# Look for infinite scroll components
rg "InfiniteScroll" --type-add 'ts:*.{ts,tsx}' -t ts
# Look for useInfiniteQuery usage
rg "useInfiniteQuery" --type-add 'ts:*.{ts,tsx}' -t ts -C 5
Length of output: 46967
…ps://github.com/JavidSumra/care_fe into issues/9188/discussion-note-infinite-scroll-bug
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/PatientNotesList.tsx (2)
Line range hint
13-22: Improve type safety by replacing 'any' typesThe interface has some props using the
anytype which could lead to type-safety issues.Consider this improvement:
interface PatientNotesProps { state: PatientNoteStateType; - setState: any; + setState: React.Dispatch<React.SetStateAction<PatientNoteStateType>>; patientId: string; facilityId: string; reload?: boolean; - setReload?: any; + setReload?: (value: boolean) => void; thread: PatientNotesModel["thread"]; setReplyTo?: (reply_to: PatientNotesModel | undefined) => void; }
42-47: Optimize state updates and add cleanupThe current state management could be improved to prevent unnecessary rerenders and potential memory leaks.
Consider these improvements:
useEffect(() => { + if (notes === state.notes) return; setState((prevState: any) => ({ ...prevState, notes, })); + return () => { + // Cleanup function to prevent updates on unmounted component + }; }, [notes, setState, state.notes]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/components/Facility/PatientConsultationNotesList.tsx(4 hunks)src/components/Facility/PatientNotesList.tsx(4 hunks)src/components/Facility/PatientNotesSlideover.tsx(3 hunks)src/components/Patient/PatientDetailsTab/Notes.tsx(2 hunks)src/components/Patient/PatientNotes.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/Patient/PatientDetailsTab/Notes.tsx
- src/components/Facility/PatientConsultationNotesList.tsx
- src/components/Facility/PatientNotesSlideover.tsx
- src/components/Patient/PatientNotes.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Facility/PatientNotesList.tsx (1)
Learnt from: UdaySagar-Git
PR: ohcnetwork/care_fe#9298
File: src/components/Facility/PatientNotesDetailedView.tsx:147-188
Timestamp: 2024-12-05T22:41:24.173Z
Learning: In the `PatientNotesDetailedView` component, child notes (`state.child_notes`) are fetched along with the parent note in a single query, so a separate loading state for child notes is not required.
🔇 Additional comments (3)
src/components/Facility/PatientNotesList.tsx (3)
Line range hint 49-65: LGTM: Loading state and render logic are well implemented
The loading state implementation is appropriate for infinite scrolling, showing the spinner only when there's no initial data. The props passed to DoctorNote component are correct and aligned with the requirements.
26-40: 🛠️ Refactor suggestion
Enhance useInfiniteQuery implementation for better error handling and pagination
The current implementation lacks error handling and uses a static offset.
As previously suggested, consider these improvements:
const {
items: notes,
loading,
fetchNextPage,
hasMore,
+ error,
} = useInfiniteQuery<PatientNotesModel>(routes.getPatientNotes, {
deduplicateBy: (note) => note.id,
query: {
thread,
- offset: 0,
+ offset: (page) => page * 20,
},
pathParams: {
patientId: props.patientId,
},
+ retries: 3,
+ onError: (error) => {
+ console.error('Failed to fetch notes:', error);
+ },
});26-40: Verify fix for unexpected page reloads
Let's verify if this implementation resolves the reported issue of unexpected page reloads after the 15th message.
✅ Verification successful
The search results show that there are no conflicting implementations of patient notes fetching using useInfiniteQuery. Additionally, the scroll-related code found in the codebase is primarily for UI interactions (like scrolling to form sections or error messages) and doesn't interfere with the infinite scrolling implementation in PatientNotesList.
Let's verify one more aspect to ensure the implementation is complete:
Let's check the implementation in the related components to ensure the infinite scrolling is properly integrated:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Implementation correctly addresses page reload issues
The implementation in PatientNotesList.tsx properly handles infinite scrolling through useInfiniteQuery with:
- Correct deduplication of notes using unique IDs
- Proper offset handling in the query
- No conflicting scroll manipulations that could interfere with the loading mechanism
- Consistent implementation across all note-related components
The changes effectively resolve the unexpected page reload issues by:
- Using a proper infinite scrolling mechanism instead of manual pagination
- Maintaining scroll position through the built-in functionality of useInfiniteQuery
- Avoiding any direct DOM manipulation for scroll handling in the notes list
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct DOM manipulation or scroll position updates
rg -g '*.{ts,tsx}' -A 3 'window\.scroll|scrollTo|scrollTop'
# Look for other implementations of note fetching that might conflict
ast-grep --pattern 'useInfiniteQuery.*getPatientNotes'
Length of output: 7050
Script:
#!/bin/bash
# Check if there are any other pagination or offset-related implementations for patient notes
rg -g '*.{ts,tsx}' -A 3 'offset.*getPatientNotes|getPatientNotes.*offset'
# Look for any other components that might be handling patient notes listing
fd -e tsx -e ts | xargs rg -l "PatientNotesModel"
Length of output: 462
Script:
#!/bin/bash
# Check the implementation in related components
cat src/components/Patient/PatientNotes.tsx src/components/Patient/PatientDetailsTab/Notes.tsx src/components/Facility/PatientConsultationNotesList.tsx
# Check for any pagination-related props or state
ast-grep --pattern 'interface $_ {
$$$
offset
$$$
}'
Length of output: 14787
…issues/9188/discussion-note-infinite-scroll-bug
…ps://github.com/JavidSumra/care_fe into issues/9188/discussion-note-infinite-scroll-bug
…issues/9188/discussion-note-infinite-scroll-bug
…ps://github.com/JavidSumra/care_fe into issues/9188/discussion-note-infinite-scroll-bug
nihal467
added
needs testing
needs review
and removed
test failed
changes required
labels
Proposed Changes
fetchNotefunction to resolve unexpected page reloads while scrolling.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
useInfiniteQuery.