Skip to content
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.

Sign up for GitHub

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

[CP Staging] Fix regression marking as read report that is not focused #35292

Closed
wants to merge 3 commits into from
Closed

[CP Staging] Fix regression marking as read report that is not focused #35292

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e635b3d
run the effect on only top most report
FitseTLT Jan 27, 2024
3b1985d
update to Navigation getTopmostreportid
FitseTLT Jan 28, 2024
64f5f2f
update early return
FitseTLT Jan 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/pages/home/report/ReportActionsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import useReportScrollManager from '@hooks/useReportScrollManager';
import useThemeStyles from '@hooks/useThemeStyles';
import compose from '@libs/compose';
import DateUtils from '@libs/DateUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import Visibility from '@libs/Visibility';
Expand Down Expand Up @@ -413,10 +414,15 @@ function ReportActionsList({
}, [calculateUnreadMarker, report.lastReadTime, messageManuallyMarkedUnread]);

const onVisibilityChange = useCallback(() => {
if (report.reportID !== Navigation.getTopmostReportId()) {
Copy link
Contributor

@tienifr tienifr Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FitseTLT @abdulrahuman5196 Sorry for chiming in but I think it's better to move this check to before this line, to avoid redundant orphaned listeners that are of no use, especially if the user has a ton of chat.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FitseTLT I think its fine to add the check at both places(the current one and the focus check suggested) which provide more rigid check and avoid stale listeners. Any reason we don't want the focus check? @FitseTLT

Copy link
Contributor

@tienifr tienifr Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, we don't render more than 5 report screens on the stack so no serious performance loss with the
current approach.

@FitseTLT it's not serious performance loss but I also don't see why we should keep those stale listeners, while removing listeners if not in use doesn't have any drawback and is definitely better (although not hugely better, but a lot of small optimizations will make the app smoother in aggregate)

cc @abdulrahuman5196

return;
}

if (!Visibility.isVisible()) {
userInactiveSince.current = DateUtils.getDBTime();
return;
}

// In case the user read new messages (after being inactive) with other device we should
// show marker based on report.lastReadTime
const newMessageTimeReference = userInactiveSince.current > report.lastReadTime ? userActiveSince.current : report.lastReadTime;
Expand Down
Loading