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.

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

Only refresh notifications once with notify push #1153

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Feb 23, 2022

Steps

  1. Setup notify_push
  2. Open n tabs with the nextcloud instance
  3. occ notification:test-push admin

Before

n tabs fetch the notification endpoint
Company call count of requests from browser: 65

After

Only the tab that was loaded last will fetch notifications and the others read from shared browser storage again
Company call count of IPs from browser: 27

After math

65 - 27 = 38 requests can be saved

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

/backport to stable23

@nickvergessen
Copy link
Member Author

/backport to stable22

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

only 1 tabs makes the notification request after push as expected

@nickvergessen nickvergessen merged commit 3b4a107 into master Feb 23, 2022
@nickvergessen nickvergessen deleted the bugfix/noid/only-refresh-notifications-once-with-notify_push branch February 23, 2022 16:40
@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@nickvergessen
Copy link
Member Author

/backport a8ed08e 61b0378 to stable23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants