-
Notifications
You must be signed in to change notification settings - Fork 277
Description
Details extracted from my comment on the other issue:
may hit GitHub’s rate limits
I only had a quick skim through the code (assisted by GitHub Copilot GPT-5), but from what I saw, it looks like the app currently doesn't make use of headers such as ETags,
Last-Modified
/If-Modified-Since
,X-Poll-Interval
, etc:
- https://docs.github.com/en/rest/activity/notifications
Notifications are optimized for polling with the
Last-Modified
header. If there are no new notifications, you will see a304 Not Modified
response, leaving your current rate limit untouched. There is anX-Poll-Interval
header that specifies how often (in seconds) you are allowed to poll. In times of high server load, the time may increase. Please obey the header.Using these aligns with GitHub's best practices for using their API; and by doing so, if the content isn't modified, it won't count against the primary rate limit:
- https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api#use-conditional-requests-if-appropriate
Use conditional requests if appropriate
Most endpoints return an
etag
header, and many endpoints return alast-modified
header. You can use the values of these headers to make conditionalGET
requests. If the response has not changed, you will receive a304 Not Modified
response. Making a conditional request does not count against your primary rate limit if a304
response is returned and the request was made while correctly authorized with anAuthorization
header.
listNotificationsForAuthenticatedUser
seems to be the main function that lists notifications:gitify/src/renderer/utils/api/client.ts
Lines 62 to 81 in 56b98ec
/** * List all notifications for the current user, sorted by most recently updated. * * Endpoint documentation: https://docs.github.com/en/rest/activity/notifications#list-notifications-for-the-authenticated-user */ export function listNotificationsForAuthenticatedUser( account: Account, settings: SettingsState, ): AxiosPromise<Notification[]> { const url = getGitHubAPIBaseUrl(account.hostname); url.pathname += 'notifications'; url.searchParams.append('participating', String(settings.participating)); return apiRequestAuth( url.toString() as Link, 'GET', account.token, {}, settings.fetchAllNotifications, ); } Which calls
apiRequestAuth
as the main underlying authenticated API helper function:gitify/src/renderer/utils/api/request.ts
Lines 30 to 81 in 56b98ec
/** * Perform an authenticated API request * * @param url * @param method * @param token * @param data * @param fetchAllRecords whether to fetch all records or just the first page * @returns */ export async function apiRequestAuth( url: Link, method: Method, token: Token, data = {}, fetchAllRecords = false, ): AxiosPromise | null { const headers = await getHeaders(url, token); if (!fetchAllRecords) { return axios({ method, url, data, headers }); } let response: AxiosResponse | null = null; let combinedData = []; try { let nextUrl: string | null = url; while (nextUrl) { response = await axios({ method, url: nextUrl, data, headers }); // If no data is returned, break the loop if (!response?.data) { break; } combinedData = combinedData.concat(response.data); // Accumulate data nextUrl = getNextURLFromLinkHeader(response); } } catch (err) { rendererLogError('apiRequestAuth', 'API request failed:', err); throw err; } return { ...response, data: combinedData, } as AxiosResponse; } That calls
getHeaders
:gitify/src/renderer/utils/api/request.ts
Lines 102 to 123 in 56b98ec
/** * Construct headers for API requests * * @param username * @param token * @returns */ async function getHeaders(url: Link, token?: Token) { const headers: Record<string, string> = { Accept: 'application/json', 'Cache-Control': shouldRequestWithNoCache(url) ? 'no-cache' : '', 'Content-Type': 'application/json', }; if (token) { const decryptedToken = (await decryptValue(token)) as Token; headers.Authorization = `token ${decryptedToken}`; } return headers; } Which will conditionally apply
Cache-Control: no-cache
based onshouldRequestWithNoCache
:gitify/src/renderer/utils/api/request.ts
Lines 83 to 100 in 56b98ec
/** * Return true if the request should be made with no-cache * * @param url * @returns boolean */ function shouldRequestWithNoCache(url: string) { const parsedUrl = new URL(url); switch (parsedUrl.pathname) { case '/api/v3/notifications': case '/login/oauth/access_token': case '/notifications': return true; default: return false; } } It's also worth noting that in
apiRequestAuth
, if thefetchAllRecords
param istrue
, it will loop through all the pages based ongetNextURLFromLinkHeader
.So based on all of that, Gitify will make 'heavier' requests than it needs to by default, and even more if there are multiple pages of notifications (even if they haven't changed in the last
fetchInterval
).Worked example: 60s interval, 2 accounts, each with 3 pages of notifications, no changes since last poll
Current implementation (no conditional headers,
fetchAll
on)
- Per poll: 2 accounts x 3 pages = 6 GETs, all 200 OK with full bodies.
- Requests/hour: 6 req/min x 60 = 360, all counted against the REST v3 core limit.
- Bandwidth (illustrative): if ~100 KB/page -> 6 pages/min ~= 600 KB/min → ~36 MB/hour.
Lightweight polling (
If-None-Match
orIf-Modified-Since
; obeyX-Poll-Interval
; stop on first304
)
- Per poll: 1 GET per account to the first page with conditional headers; server returns 304 Not Modified for both; no pagination attempted.
- Requests/hour: 2 req/min x 60 = 120, but 304s do not decrement the notifications rate limit per GitHub’s docs. Net core rate-limit cost ~= 0/hour when unchanged.
- Bandwidth (illustrative): headers-only ~1 KB/response -> ~2 KB/min -> ~120 KB/hour.
Net effect when nothing changes:
- Counted requests: ~360/hour -> ~0/hour.
- Total requests sent: 360 -> 120 (and could drop further if
X-Poll-Interval
> 60s).- Data transferred: on the order of tens of MB/hour -> ~0.1 MB/hour.
- Behavior on changes: only then do you get 200 OK, decrement the limit, and paginate as needed.
Edit: I see there was also this previous work, I had a quick skim through the changes in the PR, but I don't think they actually invalidate what I said above; particularly as
shouldRequestWithNoCache
seems to explicitly make/api/v3/notifications
/notifications
useno-cache
:This was also suggested as another potential option for handling caching / similar (and likely would also automatically handle things like
X-Poll-Interval
, rate limit related headers, etc:And
react-query
was mentioned a couple of times (1, 2), which is something I have worked with before and is quite nice.Originally posted by @0xdevalias in #2284 (comment)