Skip to content

fix(browser): Don't create web notifications for old notifications #1943

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
nickvergessen merged 2 commits into from
Jun 10, 2024
Merged

fix(browser): Don't create web notifications for old notifications #1943

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a54367
fix(browser): Don't create web notifications for old notifications
nickvergessen Jun 10, 2024
227b208
chore(assets): Recompile assets
nextcloud-command Jun 10, 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
Next Next commit
fix(browser): Don't create web notifications for old notifications 
Sometimes a notification got "newly mounted" while being old.
This can happen when a user has many notifications (100-1).
The UI first only loads (100-76), if any notification is then
resolved (e.g. by deleting or reading a chat), further old
notifications (75+74) would be added to the UI and triggered
a web notification (including call sound) in the past.

This threshold ID is therefore updated to only higher values,
before each pulling of notifications to ensure that we only ever
web-notify on new notifications and not newly loaded old
notifications.

Signed-off-by: Joas Schilling <[email protected]>
  • Loading branch information
@nickvergessen
nickvergessen committed Jun 10, 2024
commit 6a543679b4de8af65cc82baa233ae17ffbbbc1af
4 changes: 3 additions & 1 deletion src/Components/Notification.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,9 @@ export default {
emit('notifications:notification:received', event)
}

if (this.shouldNotify && this.$parent.$parent.$parent.showBrowserNotifications) {
if (this.shouldNotify
&& this.$parent.$parent.$parent.showBrowserNotifications
&& this.$parent.$parent.$parent.webNotificationsThresholdId < this.notificationId) {
Copy link
Collaborator

@Antreesy Antreesy Jun 10, 2024

Choose a reason for hiding this comment

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

Changes look good in general. I'm just not sure about this place:

  • you changing webNotificationsThresholdId before and right after the _fetch, so Vue components for new notifications might not be mounted yet. Wouldn't we skip this block of code for the very last notification?
Suggested change
&& this.$parent.$parent.$parent.webNotificationsThresholdId < this.notificationId) {
&& this.$parent.$parent.$parent.webNotificationsThresholdId <= this.notificationId) {

Copy link
Member Author

@nickvergessen nickvergessen Jun 10, 2024

Choose a reason for hiding this comment

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

you changing webNotificationsThresholdId before and right after the _fetch, so Vue components for new notifications might not be mounted yet.

Before was added "later" and should not be an issue.

After is no problem as the first run (!this.backgroundFetching which is wrapping it) is not generating notifications anyway. I was just afraid to remove this, in case someone would manually delete/resolve all of the initial load. Then the first background fetch would have an empty notification list and therefore there would still be sound+notifications for old things that are loaded with the first background fetch

this._createWebNotification()

if (this.app === 'spreed' && this.objectType === 'call') {
Expand Down
25 changes: 25 additions & 0 deletions src/NotificationsApp.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,23 @@ export default {
userStatus: null,
tabId: null,

/**
* Notifications older than this ID will not do a web notification.
*
* Sometimes a notification got "newly mounted" while being old.
* This can happen when a user has many notifications (100-1).
* The UI first only loads (100-76), if any notification is then
* resolved (e.g. by deleting or reading a chat), further old
* notifications (75+74) would be added to the UI and triggered
* a web notification (including call sound) in the past.
*
* This threshold ID is therefore updated to only higher values,
* before each pulling of notifications to ensure that we only ever
* web-notify on new notifications and not newly loaded old
* notifications.
*/
webNotificationsThresholdId: 0,

/** @type {number} */
pollIntervalBase: 30000, // milliseconds
/** @type {number} */
Expand Down Expand Up @@ -366,6 +383,10 @@ export default {
* Performs the AJAX request to retrieve the notifications
*/
async _fetch() {
if (this.notifications.length && this.notifications[0].notificationId > this.webNotificationsThresholdId) {
this.webNotificationsThresholdId = this.notifications[0].notificationId
}

const response = await getNotificationsData(this.tabId, this.lastETag, !this.backgroundFetching, this.hasNotifyPush)

if (response.status === 204) {
Expand All @@ -380,6 +401,10 @@ export default {
console.debug('Got notification data, restoring default polling interval.')
this._setPollingInterval(this.pollIntervalBase)
this._updateDocTitleOnNewNotifications(this.notifications)

if (!this.backgroundFetching && this.notifications.length) {
this.webNotificationsThresholdId = this.notifications[0].notificationId
}
} else if (response.status === 304) {
// 304 - Not modified
this._setPollingInterval(this.pollIntervalBase)
Expand Down