Skip to content

fix: notification count for empty reports #30133

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

Merged
Gonals merged 13 commits into from
Dec 5, 2023
Merged

fix: notification count for empty reports #30133

Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b0c9b95
fix: unread logic
allroundexperts Oct 21, 2023
90c915b
fix: empty condition
allroundexperts Oct 21, 2023
15294be
feat: mimic lhn
allroundexperts Oct 22, 2023
7fba759
fix: remove unused variable
allroundexperts Oct 24, 2023
208d572
Merge branch 'main' into fix-28536
allroundexperts Oct 24, 2023
d5391d9
Merge branch 'main' into fix-28536
allroundexperts Nov 16, 2023
04119fd
fix lint errors
allroundexperts Nov 16, 2023
b5987fe
merge with main
allroundexperts Nov 29, 2023
1cdb174
lint fix
allroundexperts Nov 29, 2023
50580cb
fix: sync unread count with lhn
allroundexperts Nov 30, 2023
5bff1bd
fix: unit tests
allroundexperts Nov 30, 2023
59cfe6f
fix: added comment
allroundexperts Nov 30, 2023
bbe1a98
fix: remove report param since its stale
allroundexperts Nov 30, 2023
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
41 changes: 16 additions & 25 deletions src/libs/UnreadIndicatorUpdater/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,29 @@
import {InteractionManager} from 'react-native';
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import * as ReportUtils from '@libs/ReportUtils';
import CONST from '@src/CONST';
import Navigation, {navigationRef} from '@navigation/Navigation';
import ONYXKEYS from '@src/ONYXKEYS';
import updateUnread from './updateUnread/index';

let previousUnreadCount = 0;
let allReports = [];

const triggerUnreadUpdate = () => {
const currentReportID = navigationRef.isReady() ? Navigation.getTopmostReportId() : '';

@jakub-trzebiatowski jakub-trzebiatowski Nov 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have you tested this '' case? I understand the defence-in-depth strategy, but I'm not convinced by this specific implementation of it.

Maybe...

const currentReportID = navigationRef.isReady() ? Navigation.getTopmostReportId() : null;

const unreadReports = _.filter(allReports, (report) => {
    const isAccessible = currentReportID !== null ? ReportUtils.shouldReportBeInOptionList(report, currentReportID) : true;
    
    return ReportUtils.isUnread(report) && isAccessible;
});

@jakub-trzebiatowski jakub-trzebiatowski Nov 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or : false, depending on what we want to do in the corner case

@allroundexperts allroundexperts Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is in accordance with how we're saving currentReportID in CurrentReportIdContext

@jakub-trzebiatowski jakub-trzebiatowski Nov 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok


// We want to keep notification count consistent with what can be accessed from the LHN list
const unreadReports = _.filter(allReports, (report) => ReportUtils.isUnread(report) && ReportUtils.shouldReportBeInOptionList(report, currentReportID));
updateUnread(_.size(unreadReports));
};

Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (reportsFromOnyx) => {
if (!reportsFromOnyx) {
return;
}

/**
* We need to wait until after interactions have finished to update the unread count because otherwise
* the unread count will be updated while the interactions/animations are in progress and we don't want
* to put more work on the main thread.
*
* For eg. On web we are manipulating DOM and it makes it a better candidate to wait until after interactions
* have finished.
*
* More info: https://reactnative.dev/docs/interactionmanager
*/
InteractionManager.runAfterInteractions(() => {
const unreadReports = _.filter(reportsFromOnyx, (report) => ReportUtils.isUnread(report) && report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN);

@jakub-trzebiatowski jakub-trzebiatowski Nov 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why was the report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN condition removed?

@allroundexperts allroundexperts Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because we wanted to sync this exactly with the LHN.

@jakub-trzebiatowski jakub-trzebiatowski Nov 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know, did we?

I summed it up before as...

"A notification count should only include threads that are accessible, i.e. can be navigated to".

...in other words...

"A notification count should not include threads that are inaccessible, i.e. cannot be navigated to".

...and I stand with this. It's my personal summary based on my understanding of the issue.

Notification preferences are a user-accessible feature:

image

We don't want to break this, right? It sounds like a different topic.

@allroundexperts allroundexperts Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is something different. By setting this preference, you're setting if you want to receive a notification for a new message. Changing this won't really effect the count.

@jakub-trzebiatowski jakub-trzebiatowski Nov 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In other words, do you suggest that the old code checking for report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN was a mistake (an unnecessary / irrelevant check)?

Or maybe that the user setting I added a screenshot of is not actually related to report.notificationPreference?

@allroundexperts allroundexperts Dec 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@marcaaron Im OoO for two more days. I'll raise a PR for this after then if that's fine.

@jakub-trzebiatowski jakub-trzebiatowski Dec 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a GH issue for this mentioned regression?

@gijoe0295 gijoe0295 Dec 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think #33506 could be regression from this PR.

@jakub-trzebiatowski jakub-trzebiatowski Jan 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@allroundexperts

Im OoO for two more days. I'll raise a PR for this after then if that's fine.

Would you provide an update?

We can move the further discussion to #33506

@allroundexperts allroundexperts Jan 7, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR created already!

const unreadReportsCount = _.size(unreadReports);
if (previousUnreadCount !== unreadReportsCount) {
previousUnreadCount = unreadReportsCount;
updateUnread(unreadReportsCount);
}
});
allReports = reportsFromOnyx;
triggerUnreadUpdate();
},
});

navigationRef.addListener('state', () => {
triggerUnreadUpdate();
});