-
Notifications
You must be signed in to change notification settings - Fork 441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Inifinite Load issue in notes #9190
base: develop
Are you sure you want to change the base?
Fix Inifinite Load issue in notes #9190
Conversation
WalkthroughThe changes made in this pull request focus on improving the loading state management and control flow within the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 (1.23.1)
src/components/Facility/PatientConsultationNotesList.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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-window
orreact-virtualized
to efficiently render large lists- Implement scroll position restoration using
ScrollRestoration
fromreact-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-window
for 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.
fetchNotes().then(() => { | ||
setIsLoading(false); | ||
setReload(false); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding error handling and preventing race conditions
While moving state updates into Promise.then() is good, we should handle errors and prevent race conditions during concurrent fetches.
Consider this improvement:
- fetchNotes().then(() => {
- setIsLoading(false);
- setReload(false);
- });
+ let isMounted = true;
+ fetchNotes()
+ .then(() => {
+ if (isMounted) {
+ setIsLoading(false);
+ setReload(false);
+ }
+ })
+ .catch((error) => {
+ if (isMounted) {
+ console.error('Failed to fetch notes:', error);
+ setIsLoading(false);
+ setReload(false);
+ }
+ });
+ return () => {
+ isMounted = false;
+ };
Committable suggestion skipped: line range outside the PR's diff.
fetchNotes().then(() => { | ||
setIsLoading(false); | ||
setReload?.(false); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling to prevent UI from being stuck in loading state
While moving the loading state update to the promise resolution is good, we should also handle errors to prevent the UI from being stuck in a loading state if the fetch fails.
Consider adding error handling:
fetchNotes().then(() => {
setIsLoading(false);
setReload?.(false);
-});
+}).catch((error) => {
+ setIsLoading(false);
+ setReload?.(false);
+ // Consider adding error state management here
+});
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fetchNotes().then(() => { | |
setIsLoading(false); | |
setReload?.(false); | |
}); | |
fetchNotes().then(() => { | |
setIsLoading(false); | |
setReload?.(false); | |
}).catch((error) => { | |
setIsLoading(false); | |
setReload?.(false); | |
// Consider adding error state management here | |
}); |
Proposed Changes
fetchNote
function to resolve unexpected page reloads while scrolling.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes