Stop reporting a notification immediately after it's viewed #106

not-an-aardvark posted onGitHub

The following happens to me somewhat often:

The notifier icon updates with a "1", letting me know that I have a notification
I click on the icon, view the notification, and go back to what I was doing before
45 seconds later, I notice a "1" on the notifier icon, and I click it again thinking I have another notification, only to realize that the icon is out-of-date. It's still letting me know about the notification that I just viewed, and I don't actually have any new notifications.

This is only a mild issue because the extension re-fetches from GitHub every minute and realizes that I don't have any new notifications. However, it would be nice if the extension could prevent it by doing something like this:

For each notification, store the corresponding URL and a timestamp of when the notification was fetched.
When the user opens a tab that matches the URL of one of the notifications, immediately update the counter in the notifier icon to reflect the fact that the notification is no longer "new".
Clear the stored URLs and repeat the process whenever a new batch of notifications is fetched from GitHub.

Note: This issue has a bounty, so it's expected that you are an experienced programmer and that you give it your best effort if you intend to tackle this. Don't forget, if applicable, to add tests, docs (double-check for typos), and update TypeScript definitions. And don't be sloppy. Review your own diff multiple times and try to find ways to improve and simplify your code. Instead of asking too many questions, present solutions. The point of an issue bounty is to reduce my workload, not give me more. Include a 🦄 in your PR description to indicate that you've read this. Thanks for helping out 🙌 - @sindresorhus

@issuehunt has funded $60.00 to this issue.

Submit pull request via IssueHunt to receive this reward.
Want to contribute? Chip in to this issue via IssueHunt.
Checkout the IssueHunt Issue Explorer to see more funded issues.
Need help from developers? Add your repository on IssueHunt to raise funds.

posted by issuehunt-app[bot] almost 6 years ago

I'm planning to work on this and I have couple of points to mention here

1) Right now tabs permission is optional, for this feature to work we need tabs permission(to read the url), should we move tabs permission to required permissions?

If not, we need to ask the user explicitly permission, when should we ask the user for this permission? 

here is the link to optional permission documentation https://developer.chrome.com/apps/permissions

I think we should move the `tabs` permission to require permission as it will be an important feature

2) Right now, we are only pulling one notification from api and we are calculating count based on the headers from the response.

For this feature to work, we need to pull maximum number of notifications from the api which are 100 per call.

One edge case might be, some user might have more than 100 notifications and we only save latest 100 notifications in local storage and when user opens 101 notification we might not be updating the badge text, as we are only saving latest 100 notifications.

@sindresorhus @notlmn any thoughts here?

posted by sasikanth513 almost 6 years ago

I don't think you need to store any data for this feature, you could match the tab url on browser.tabs.onTabUpdated callback listener to match with a GitHub like URL and update the notification count.

I haven't tested this, but sounds like a solution that doesn't require you to store any large amounts of data.

And about the tabs permission, it has to stay optional, many users don't like the idea of an extension being able to read your browser history. This should work only when the tabs permission is granted.

posted by notlmn almost 6 years ago

Thanks for the response @notlmn. Couple of thoughts

We don't need to save large amount of data, we just need to save the links to all the notifications.

For e.g, If I have a notification related to this thread https://github.com/sindresorhus/camelcase-keys/issues/32, When user opens that link on their browser, we should know that there is active notification related to this thread and we should then reduce the counter by 1. For this to work, we need to pull the notification links from api and save them in localstorage.

Making tabs permission optional makes sense. For users I think there is no manual way to go and give tabs permission. We need to ask the users for this permission at some point using the api I mentioned here https://developer.chrome.com/apps/permissions.

When should we ask for this optional permission from the user? If user gives permission, we will give this feature otherwise the flow will continue without any issues.

posted by sasikanth513 almost 6 years ago

And about the tabs permission, it has to stay optional, many users don't like the idea of an extension being able to read your browser history. This should work only when the tabs permission is granted.

:+1: But we should clearly document what the user misses out on if they don't grant the permission.

posted by sindresorhus almost 6 years ago

I absolutely agree with the user privacy here.

I think we can give an option to the user in options page. At the end of the form, show a button/link asking for tabs permission(and revoke permission if already given) with the note saying, why we need that permission.

Any thoughts on this flow? Also thoughts on saving links in local storage?

posted by sasikanth513 almost 6 years ago

We don't need to save large amount of data, we just need to save the links to all the notifications. For e.g, If I have a notification related to this thread https://github.com/sindresorhus/camelcase-keys/issues/32, When user opens that link on their browser, we should know that there is active notification related to this thread and we should then reduce the counter by 1. For this to work, we need to pull the notification links from api and save them in localstorage.

Ideally, the extension should not store anything. My argument is that when a user visits a webpage, match its URL with a regex something like https://{host}/user/repo/[issues|pull]/id, or the /commit/sha1 URL for some repo (likely URLs in a notification). Then you could query the GitHub API again to get the new count for the notifications, by calling update() in background.js. This {host} property comes from getHostname() function in api.js.

And also this code should be place in a new file, with its own tests added along with it. The naming and implementation is left to the one implementing it.

Making tabs permission optional makes sense. For users I think there is no manual way to go and give tabs permission. We need to ask the users for this permission at some point using the api I mentioned here developer.chrome.com/apps/permissions. When should we ask for this optional permission from the user? If user gives permission, we will give this feature otherwise the flow will continue without any issues.

We already request tabs permission for Reuse notifications tabs when possible option. Look at the implementation for it in options.js on how we dynamically request permissions like tabs and notifications.

posted by notlmn almost 6 years ago