regression: fix jump from pseudo message lists #35880
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| @@ -14,26 +15,35 @@ export const useGetMore = (rid: string, atBottomRef: MutableRefObject<boolean>) | |||
|
|
|||
| const refValue = ref.current; | |||
|
|
|||
| const handleScroll = withThrottling({ wait: 100 })((event) => { | |||
| const handleScroll = withThrottling({ wait: 100 })(async (event) => { | |||
| const lastScrollTopRef = event.target.scrollTop; | |||
There was a problem hiding this comment.
async (event: React.UIEvent<HTMLElement>) => {
const target = event.target;
if (!(target instanceof HTMLElement)) {
return;
}
const lastScrollTopRef = target.scrollTop;
const height = target.clientHeight;
// ...
} else if (hasMoreNext === true && Math.ceil(lastScrollTopRef) >= target.scrollHeight - height) {Accessing properties like scrollTop, clientHeight, and scrollHeight directly from event.target assumes it's an Element with these properties, which can lead to runtime errors if the target is not the expected type.
This issue appears in multiple locations:
- apps/meteor/client/views/room/body/hooks/useGetMore.ts: Lines 19-35
- apps/meteor/client/views/room/body/hooks/useGetMore.ts: Lines 19-35
Please add a type check (e.g.,instanceof HTMLElement) at the beginning of the handler to validateevent.targetand prevent potential runtime errors.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await RoomHistoryManager.getMore(rid); | ||
|
|
||
| flushSync(() => { | ||
| RoomHistoryManager.restoreScroll(rid); | ||
| }); |
There was a problem hiding this comment.
try {
await RoomHistoryManager.getMore(rid);
flushSync(() => {
RoomHistoryManager.restoreScroll(rid);
});
} catch (error) {
console.error('Failed to get more room history:', error);
// Optional: Implement user feedback mechanism
}The await RoomHistoryManager.getMore(rid) call could potentially reject (e.g., due to network issues), leading to unhandled promise rejections.
This issue appears in multiple locations:
- apps/meteor/client/views/room/body/hooks/useGetMore.ts: Lines 30-34
- apps/meteor/client/views/room/body/hooks/useGetMore.ts: Lines 30-34
Please wrap the asynchronous operation and the subsequentflushSynccall within atry...catchblock to handle potential errors gracefully.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| refValue.addEventListener('scroll', handleScroll, { | ||
| passive: true, | ||
| }); | ||
|
|
||
| return () => { | ||
| handleScroll.cancel(); | ||
| refValue.removeEventListener('scroll', handleScroll); |
There was a problem hiding this comment.
let scrollEventListener = null;
const handleScroll = withThrottling({ wait: 100 })(async (event) => { /* ... */ });
scrollEventListener = handleScroll; // Store a reference to the throttled function
refValue.addEventListener('scroll', scrollEventListener, { passive: true });
// ...
return () => {
if (scrollEventListener) {
refValue.removeEventListener('scroll', scrollEventListener);
}
};While the original event listener is removed in the cleanup function, the addition of the throttled scroll listener introduces a potential issue. The handleScroll.cancel() method only cancels the throttled call, not the event listener itself. This can lead to memory leaks or unintended behavior if the component re-renders. Ensure the event listener attached to the throttled function is properly removed in the cleanup function.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #35880 +/- ##
========================================
Coverage 61.14% 61.14%
========================================
Files 3009 3009
Lines 71554 71573 +19
Branches 16378 16381 +3
========================================
+ Hits 43751 43765 +14
- Misses 24836 24842 +6
+ Partials 2967 2966 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
regression/load-more
branch
from
2daa7e3 to
e746d93
Compare
Proposed changes (including videos or screenshots)
Introduced here: #35818
Issue(s)
Steps to test or reproduce
Further comments
CORE-1098
CORE-1099
This pull request addresses a regression issue related to message list navigation in the Rocket.Chat application. The changes focus on improving the user experience when loading older messages by implementing scroll position saving and restoration within the
RoomHistoryManager. Specifically, the current scroll position is saved when older messages are loaded using thegetMoremethod, and a newrestoreScrollmethod is introduced to adjust for content height changes. Additionally, aloaded-messagesevent is emitted after messages are loaded in bothgetMoreandgetMoreNext, and a utility functionwaitAfterFlushis added.In the
useJumpToMessagehook, modifications are made to enhance listRef handling and message scrolling behavior. TheuseGetMorehook is refactored to useasync/awaitfor fetching older messages, introducesflushSyncfor smoother scroll restoration, and optimizes performance by addingpassive: trueto the scroll listener. The throttled handler is also ensured to be cancelled on unmount. These improvements aim to enhance the history loading mechanism, although further enhancements in type safety and error handling are suggested.