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

feat(api): use conditional requests and fetch all inbox notifications #1414

Merged
merged 18 commits into from
Sep 13, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented Jul 26, 2024

Closes #1124
Closes #356

Update our API Request With Auth function to:

  • support paginated endpoints and unpaginating response data
  • conditionally set Cache-Control request header depending on the api url
    • for /notifications we do not want to cache request/response (current behavior)
    • for all other api requests, we use the default browser caching based on the GitHub API response (60 seconds)

Appreciate some hands to test this branch locally before merging 🙏

@github-actions github-actions bot added the enhancement New feature or enhancement to existing functionality label Jul 26, 2024
@setchy setchy marked this pull request as ready for review July 26, 2024 19:47
@setchy setchy changed the title feat(api): use conditional requests and fetch all notification inboxes feat(api): use conditional requests and fetch all notifications Jul 26, 2024
@setchy setchy changed the title feat(api): use conditional requests and fetch all notifications feat(api): use conditional requests and fetch all inbox notifications Jul 26, 2024
@setchy setchy added this to the Release 5.12.0 milestone Jul 28, 2024
src/utils/api/utils.ts Outdated Show resolved Hide resolved
@setchy
Copy link
Member Author

setchy commented Aug 2, 2024

@bmulholland @afonsojramos - thoughts on whether we need to place a ceiling on the total number of notifications we unpaginate, or leave as-is (fetch all pages)?

I've tested with ~650 notifications and it seems to be fine. Maybe we add a ceiling only if there are reports of rate-limiting?

@setchy
Copy link
Member Author

setchy commented Aug 2, 2024

Moving back to draft as the tests require some rework after merging #1419

@setchy setchy marked this pull request as draft August 2, 2024 21:00
@bmulholland
Copy link
Collaborator

thoughts on whether we need to place a ceiling on the total number of notifications we unpaginate, or leave as-is (fetch all pages)?

Maybe just leave it, but add a way to report to the user "you have too many notifications, it hits rate limits, you're on your own"? :)

Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Sep 6, 2024

Finally ready for this to be considered for merging. Clearing the previous approval(s) since it's been sitting for a little while

@setchy setchy marked this pull request as ready for review September 6, 2024 19:42
@bmulholland
Copy link
Collaborator

Will test locally. To start, I see this in console:

13:50:17.163 › Error occurred while fetching details for PullRequest notification: feat(lorem): add character length parameter to lorem.text TypeError: Cannot read properties of null (reading 'login')

@setchy
Copy link
Member Author

setchy commented Sep 9, 2024

Will test locally. To start, I see this in console:

13:50:17.163 › Error occurred while fetching details for PullRequest notification: feat(lorem): add character length parameter to lorem.text TypeError: Cannot read properties of null (reading 'login')

🤔 - that's pretty unusual

@setchy
Copy link
Member Author

setchy commented Sep 9, 2024

I've been running it locally for the past week and haven't had any console log entries like that

@setchy
Copy link
Member Author

setchy commented Sep 9, 2024

@brandonweiss - can you check if you're getting the above login console error using either main or the latest release please 🙏

My best guess is that PR, for some reason, doesn't have a PR user

gitify/src/utils/subject.ts

Lines 280 to 285 in fa77c99

user: {
login: prCommentUser?.login ?? pr.user.login,
html_url: prCommentUser?.html_url ?? pr.user.html_url,
avatar_url: prCommentUser?.avatar_url ?? pr.user.avatar_url,
type: prCommentUser?.type ?? pr.user.type,
},

Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Sep 9, 2024

Added a new Notification Setting to control fetching all unread inbox notifications, just in case

Screenshot 2024-09-09 at 2 36 09 PM

@bmulholland
Copy link
Collaborator

@setchy the console message may well be unrelated, unfortunately I only had that PR in my notifications exactly while testing the branch. I think you're right, though: the PR author deleted their account.

@bmulholland
Copy link
Collaborator

FYI, I've been using this branch for the last day and it seems to be working well for me :)

@setchy
Copy link
Member Author

setchy commented Sep 12, 2024

FYI, I've been using this branch for the last day and it seems to be working well for me :)

Likewise - do we feel comfortable enough to merge this change? Itching to ship the next release

src/components/settings/NotificationSettings.tsx Outdated Show resolved Hide resolved
src/components/settings/NotificationSettings.tsx Outdated Show resolved Hide resolved
setchy and others added 2 commits September 13, 2024 08:35
Co-authored-by: Afonso Jorge Ramos <afonsojorgeramos@gmail.com>
Co-authored-by: Afonso Jorge Ramos <afonsojorgeramos@gmail.com>
@setchy setchy merged commit b90814d into main Sep 13, 2024
12 checks passed
@setchy setchy deleted the feature/conditional-request branch September 13, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Response caching with conditional requests Increase the limit of notifications
3 participants