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

Persist notifications cache #97

Merged
merged 19 commits into from
Dec 29, 2020
Merged

Persist notifications cache #97

merged 19 commits into from
Dec 29, 2020

Conversation

sirbrillig
Copy link
Owner

@sirbrillig sirbrillig commented Dec 28, 2020

Currently when notifications are fetched, we request both read and unread notifications from the server. The github API has a maximum of 100 notifications that can be retrieved per page at one time. Let's say that you have 200 notifications. Within the first 100 you have 5 unread, and within the second 100, you have 2 unread. In this situation, the server response (and therefore gitnews) will only show the 5 most recent unread and you'll never see the 2 unread in the second 100 because it's on a page we do not fetch.

Furthermore, if a notification is marked as unread, since the github API does not actually support that action, we mark our local cache of the notification instead; if that notification ever passes beyond the first 100 notifications returned by the server, then our local cache of the notification will effectively expire, making it seem to disappear.

In this PR we change how fetching works to resolve these issues.

First, we make our local cache persistent beyond each fetch. We merge it with the most recent response from the server so that we never remove a local notification we've cached even if the server doesn't have it any more. Since keeping notifications indefinitely is not sustainable either, we prune our cache ourselves based on a large interval of inactivity on the notification itself rather than the number of notifications after it.

Second, we change fetching itself to have two modes of operation. Most fetches will now only ask the server for unread notifications. This way you'd have to have more than 100 unread notifications before some would not appear (this is still an issue that will need to be addressed somehow but should be less of an issue than it was previously since that 100 will no longer include read notifications). We will still fetch unread notifications on our first fetch (or any time there are no notifications in the cache) and then any time we haven't fetched read notifications for 20 minutes.

If a notification changes from unread to read after being fetched and within that 20 minute interval, it would remain in the local cache (incorrectly) as unread, so if we detect that an unread notification in our local cache is not also returned by the server during unread polling, we will assume that it has been read and mark the local cache appropriately.

If a notification is unread or has been marked unread locally, we will never remove it from our local cache, even if it is older than our local expiration interval.

Fixes #96

@sirbrillig sirbrillig marked this pull request as ready for review December 29, 2020 03:17
@sirbrillig sirbrillig merged commit 2e01b5d into master Dec 29, 2020
@sirbrillig sirbrillig mentioned this pull request Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notifications marked as unread disappear if the server stops returning it
1 participant