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

fix: Handle multiple connections correctly on dashboard #13

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

glossawy
Copy link
Contributor

react-query's built-in memoization was causing connections to the same domain to clobber each other. You would have a race condition on which set of data gets shown and it would show up twice.

The goal here is to allow multiple connections to be used and have the sum of all of their PR requests be shown in the dashboard. Since we are already storing the key plain in localStorage I don't immediately see an issue with utilizing the token as part of the react query key.

Though technically it does expose the access token to more libraries. If that's a concern we could hash the token.

@pvcnt
Copy link
Owner

pvcnt commented Jan 19, 2024

I had not considered this use case. Out of curiosity, how do you use multiple connections to the same host? How would they be configured differently?

@glossawy
Copy link
Contributor Author

I provide an example in #12 but with github finer-grained tokens you grant access to a specific user/org, giving access to a user does not mean it will show all the repositories they have access to in all their orgs. So I had to create two different tokens one for the org and one for myself.

Basically the only difference is the token used for the connection.

@pvcnt
Copy link
Owner

pvcnt commented Jan 19, 2024

That makes sense!

Copy link
Owner

@pvcnt pvcnt left a comment

Choose a reason for hiding this comment

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

Let's move forward with this for now. It's not optimal but I cannot think of anything better at the moment. And as you say, it's all stored in the browser right now - which is an explicit goal of this project (it should work all in the browser).

@@ -30,7 +30,7 @@ export default function Dashboard() {
const results = useQueries({
queries: config.sections.flatMap(section => {
return config.connections.map((connection, idx) => ({
queryKey: ['pulls', connection.host, section.search],
queryKey: ['pulls', connection.host, connection.auth, section.search],
Copy link
Owner

Choose a reason for hiding this comment

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

I was considering as an alternative using the connection name that you introduce in #12. But at the same time using the token is a better representation of the identity of the token (e.g., if we switch the names of two connections we should not reuse the cache of the other one).

@pvcnt pvcnt merged commit 0d6a32d into pvcnt:main Jan 20, 2024
1 check passed
@glossawy glossawy deleted the multi-connection-dashboard-fix branch January 21, 2024 05:05
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.

2 participants