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.

Sign up for GitHub

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

Jump to bottom

fix: Refetch username after delay when github api rate limit is triggered #891

Merged
merged 8 commits into from
Jan 17, 2025
Merged

fix: Refetch username after delay when github api rate limit is triggered #891

merged 8 commits into from
Jan 17, 2025

Conversation

EGAMAGZ
Copy link
Contributor

@EGAMAGZ EGAMAGZ commented Jan 14, 2025

Fixes #859

@EGAMAGZ EGAMAGZ marked this pull request as draft January 14, 2025 23:59
@crowlKats
Copy link
Collaborator

@EGAMAGZ is there anything blocking on continuation of this PR?

@EGAMAGZ
Copy link
Contributor Author

EGAMAGZ commented Jan 16, 2025 •
edited
Loading

I am still trying to figure out why this small change is causing a test to fail when it should not. The change resolves the issue without any problems

@EGAMAGZ EGAMAGZ marked this pull request as ready for review January 16, 2025 20:29
@EGAMAGZ
Copy link
Contributor Author

EGAMAGZ commented Jan 16, 2025 •
edited
Loading

Nevermind @crowlKats, pls, can you review it? 😅

@crowlKats
Copy link
Collaborator

@EGAMAGZ the change makes sense to me, but i am worried we will stay in a loop if there is some hard error ie a ratelimit from github; could you change the code to take that into account?

@EGAMAGZ
Copy link
Contributor Author

EGAMAGZ commented Jan 16, 2025 •
edited
Loading

Yep, I'll do it. Thanks for the feedback

@EGAMAGZ
Copy link
Contributor Author

EGAMAGZ commented Jan 16, 2025

When the rate limit is reached. Is it ok to leave the GitHubUserLink showing "loading..."?

image

@crowlKats
Copy link
Collaborator

could it maybe say "could not load GitHub username"?

@EGAMAGZ
Copy link
Contributor Author

EGAMAGZ commented Jan 16, 2025

Now it will show up like this:
image

@crowlKats
Copy link
Collaborator

@EGAMAGZ looks good, though one minor nitpick to make it a bit more readable (will land this PR after that), could you make it italic and a smaller fontsize? or instead of italic in brackets.

@EGAMAGZ
Copy link
Contributor Author

EGAMAGZ commented Jan 17, 2025 •
edited
Loading

Done 😄

image

crowlKats
crowlKats approved these changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM!

@crowlKats crowlKats added this pull request to the merge queue Jan 17, 2025
@EGAMAGZ
Copy link
Contributor Author

EGAMAGZ commented Jan 17, 2025

Thanks :D

Merged via the queue into jsr-io:main with commit 67ba749 Jan 17, 2025
6 checks passed
@EGAMAGZ EGAMAGZ deleted the fix/github-link branch January 17, 2025 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crowlKats crowlKats crowlKats approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

My account page's GitHub link is undefined
2 participants
@EGAMAGZ @crowlKats