search

hypertrons / hypertrons-crx

A browser extension for insights into GitHub, Gitee projects and developers.
https://hypercrx.cn
Apache License 2.0
357 stars 102 forks source link

feat:Added functionality to retrieve and save GitHub token (#812) #839

Closed wblxxx closed 4 months ago

wblxxx commented 4 months ago

Brief Information

This pull request is in the type of (more info about types):

Related issues (GitHub tokens):

Details

Recently, there has been a https://github.com/hypertrons/hypertrons-crx/issues/773#issuecomment-1918190670 in the community about GitHub Tokens. In fact, HyperCRX had previously introduced GitHub Tokens to increase the number of requests made to GitHub, as seen in https://github.com/hypertrons/hypertrons-crx/pull/100. However, in https://github.com/hypertrons/hypertrons-crx/issues/574, we removed it since there was no longer a frequent need to access the GitHub API.

Considering that HyperCRX may need to frequently access the GitHub API in the future, we need to reintroduce GitHub Tokens with the following specific requirements:

Provide a UI interface allowing users to input their GitHub Tokens. Securely store user's Tokens. Encapsulate a network request method that uses the GitHub Token, which will be called by other new features needing to access the GitHub API in the future.

Checklist

Others

wblxxx commented 4 months ago

Sorry for the delay in making the changes until today. Thank you for your diligent review. I have resolved all the issues in the latest version. Additionally, using the token in the input form is intuitive and user-friendly, as it enables immediate feedback on the token's validity, which I have adopted. If there are any further issues, please continue to correct me.

wxharry commented 4 months ago

Almost there! Some minor enhancements are needed before we merge it. The biggest issue it that I can't see the message, not sure what's happend. I didn't take a close at the code. Let me know if it works for you or you need more info from me.

I think I figured that out. The token won't be valid until I click save. But in the code, I can see that we are using token from the githubRequest as part of input.

wblxxx commented 4 months ago

Your suggestion makes a lot of sense. I appreciate your effort in reviewing the new version.

wblxxx commented 4 months ago

I simplified the githubApi.ts and moved the decision logic to GitHubToken.tsx.By the way,if we are only using githubToken to get the token from localStorage, we can directly use localStorage.getItem('github_token') ,the previous approach might reduce the number of accesses to localStorage in scenarios where githubRequest is called frequently, thereby improving performance.

wxharry commented 4 months ago

This is causing the bug. options has the same thing as headers, they are duplicate. I think we can just keep ...options and remove the rest.

Did you solve this? I don't get any response if I set the headers twice. Removing duplicate options works for me but the message is still not showing. Nothing is showing in the console. Do you have any ideas?

wblxxx commented 4 months ago

This is causing the bug. options has the same thing as headers, they are duplicate. I think we can just keep ...options and remove the rest.

Did you solve this? I don't get any response if I set the headers twice. Removing duplicate options works for me but the message is still not showing. Nothing is showing in the console. Do you have any ideas?

This is so strange. The day before yesterday, everything was normal when I was using and uploading the code. Yesterday, there were no message when I first clicked, but during the process of modifying the code, it naturally recovered, and everything remained normal for several hours after uploading. Just now, there were no message when I first clicked, but after having a meal, it was fine. I'm thinking about it.

wblxxx commented 4 months ago

This is causing the bug. options has the same thing as headers, they are duplicate. I think we can just keep ...options and remove the rest.

Did you solve this? I don't get any response if I set the headers twice. Removing duplicate options works for me but the message is still not showing. Nothing is showing in the console. Do you have any ideas?

I suspect that the previous fixed position of the message pop-up might have caused it to be invisible in some cases. I have adjusted it to dynamically calculate the position of the message. However, since it has always run successfully on my side, I cannot determine if the bug still exists. Please test it and provide feedback promptly. iShot_2024-07-31_15 13 29

wblxxx commented 4 months ago

I checked today, and the issue didn't happen again.

wxharry commented 4 months ago

I suspect that the previous fixed position of the message pop-up might have caused it to be invisible in some cases. I have adjusted it to dynamically calculate the position of the message. However, since it has always run successfully on my side, I cannot determine if the bug still exists. Please test it and provide feedback promptly. iShot_2024-07-31_15 13 29

Would it be easier to move it to the top of the page? because messages usually appear at the top by default. This is not a change request, you can keep it this way if that could cause the message invisible issue.

Also, did we figure out why we have duplicate options here? https://github.com/hypertrons/hypertrons-crx/pull/839#discussion_r1696302272

wblxxx commented 4 months ago

I suspect that the previous fixed position of the message pop-up might have caused it to be invisible in some cases. I have adjusted it to dynamically calculate the position of the message. However, since it has always run successfully on my side, I cannot determine if the bug still exists. Please test it and provide feedback promptly. iShot_2024-07-31_15 13 29

Would it be easier to move it to the top of the page? because messages usually appear at the top by default. This is not a change request, you can keep it this way if that could cause the message invisible issue.

Also, did we figure out why we have duplicate options here? #839 (comment)

By default, the response message appears at the top, but at that time, the user's attention is focused on the GitHubToken component area. If it appears at the top, they might not notice it, and shifting their gaze to focus on the content and then back is also not user-friendly. Therefore, I think the current interaction is better. Additionally, if our function is only to pass in the GitHub token and does not need to retain other request configurations or header information, then ...options and ...options.headers are indeed redundant. The code can be simplified, and I have made the modifications.

wxharry commented 4 months ago

LGTM. Thanks for your efforts! Looks like the branch needs formatting. Please run yarn run prettier.

wblxxx commented 4 months ago

LGTM. Thanks for your efforts! Looks like the branch needs formatting. Please run yarn run prettier.

I have always used Prettier to check the code format, and there are no issues when checking locally. The PR is also fully synchronized, so I don't know where the problem is. iShot_2024-08-02_14 25 53 iShot_2024-08-02_14 28 50

wxharry commented 4 months ago

I have always used Prettier to check the code format, and there are no issues when checking locally. The PR is also fully synchronized, so I don't know where the problem is. iShot_2024-08-02_14 25 53 iShot_2024-08-02_14 28 50

This is weird! My prettier gives three files that need to be fixed. Do you mind I adding those changes to this branch?

image
wblxxx commented 4 months ago

I have always used Prettier to check the code format, and there are no issues when checking locally. The PR is also fully synchronized, so I don't know where the problem is. iShot_2024-08-02_14 25 53 iShot_2024-08-02_14 28 50

This is weird! My prettier gives three files that need to be fixed. Do you mind I adding those changes to this branch?

image

It looks like the versions are different, but of course, it's okay.