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

Jump to bottom

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

Merged
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 all commits
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
4 changes: 2 additions & 2 deletions js/notifications-main.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/notifications-main.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions js/notifications-src_NotificationsApp_vue.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/notifications-src_NotificationsApp_vue.js.map
View file
Open in desktop

Large diffs are not rendered by default.

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 @@ -250,7 +250,7 @@
},

mounted() {
this._$el = $(this.$el)

Check warning on line 253 in src/Components/Notification.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

The global property or function $ was deprecated in Nextcloud 19.0.0

// Parents: TransitionGroup > Transition > HeaderMenu
if (typeof this.$parent.$parent.$parent.showBrowserNotifications === 'undefined') {
Expand All @@ -267,7 +267,9 @@
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

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

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 @@
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 @@ -216,12 +233,12 @@

mounted() {
this.tabId = OC.requestToken || ('' + Math.random())
this._$icon = $(this.$refs.icon)

Check warning on line 236 in src/NotificationsApp.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

The global property or function $ was deprecated in Nextcloud 19.0.0
this._oldcount = 0

// Bind the button click event
console.debug('Registering notifications container as a menu')
OC.registerMenu($(this.$refs.button), $(this.$refs.container), undefined, true)

Check warning on line 241 in src/NotificationsApp.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

The global property or function $ was deprecated in Nextcloud 19.0.0

Check warning on line 241 in src/NotificationsApp.vue

View workflow job for this annotation

GitHub Actions / NPM lint 

The global property or function $ was deprecated in Nextcloud 19.0.0

this.checkWebNotificationPermissions()

Expand Down Expand Up @@ -366,6 +383,10 @@
* Performs the AJAX request to retrieve the notifications
*/
async _fetch() {
if (this.notifications.length && this.notifications[0].notificationId > this.webNotificationsThresholdId) {
nickvergessen marked this conversation as resolved.
Show resolved Hide resolved
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 @@
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
Loading