fix: connection pending alert and alert content alignment#64120
fix: connection pending alert and alert content alignment#64120bahrmichael merged 8 commits intomainfrom
Conversation
…content alignment
| .filter(n => n.credential) | ||
| .filter(n => n.credential?.isSiteCredential === (gitHubAppKind === GitHubAppKind.SITE_CREDENTIAL)).length ?? | ||
| 0) === 0 |
There was a problem hiding this comment.
We could combine these filters to reduce the amount of loops we do for connection nodes.
There was a problem hiding this comment.
Thank you for the fix. I'm not sure how this solves the problem though. The CodeHostConnections component is used for both USER_CREDENTIAL and SITE_CREDENTIAL.
However, I'm not sure how this logic fixes it. Can you explain the solution for me?
There was a problem hiding this comment.
Sure! The problem I saw occurred when there was already a global token. When I then added a personal github app, it showed a wrong alert. Before this change, it will consider all credentials (even the site credential) as counting towards the installations that may be in progress. The approach currently used relies on checking if there are any integrations that don't have credentials yet to determine if an installation is in progress. With SRCH-798 we can deprecate this behavior, but that ticket is significantly more work.
So this component gets all the (non-commit-signing) credentials. We then add a check if it matches the gitHubAppKind condition to filter down to only user or site-admin credentials.
gitHubAppKind === GitHubAppKind.SITE_CREDENTIAL yields true if we're in the site-admin page, and that can then be compared with n.credential?.isSiteCredential.
There was a problem hiding this comment.
- If there is already a connection from another host, this may yield a false result.
| success && | ||
| gitHubAppKindFromUrl !== GitHubAppKind.COMMIT_SIGNING && | ||
| (connection?.nodes.filter(n => n.credential).length ?? 0) === 0 | ||
| (connection?.nodes.filter( |
There was a problem hiding this comment.
We need to check if the connection is a GitHub app so we can check against GitHub apps alone.
There was a problem hiding this comment.
Added a check to limit it to GitHub apps.
| return ( | ||
| nodes.filter( | ||
| n => n.externalServiceKind === ExternalServiceKind.GITHUB && n.credential?.gitHubApp?.name === appName | ||
| ).length > 0 | ||
| ) |
There was a problem hiding this comment.
This works, but we could opt for a cool array iterator available in javascript.
| return ( | |
| nodes.filter( | |
| n => n.externalServiceKind === ExternalServiceKind.GITHUB && n.credential?.gitHubApp?.name === appName | |
| ).length > 0 | |
| ) | |
| return !nodes.some(n => n.externalServiceKind === ExternalServiceKind.GITHUB && n.credential?.gitHubApp?.name === appName) |
You can read about it here.
There was a problem hiding this comment.
Right! some works too in that case.
There was a problem hiding this comment.
Yeah, just a suggestion. Feel free to discard / put off to a later PR.
THe performance improvement is negligible for now as we dont have customers with a bucket load of codehost connections.
) This is a follow up to https://github.com/sourcegraph/sourcegraph/pull/64120 so that it doesn't mix up alerts between commit signing and code hosts for site admins. ## Test plan Manual testing + CI ## Changelog <!-- OPTIONAL; info at https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c -->
2 participants
When there is already a global token, then the notice after installing a personal app is wrong. This PR handles this issue, so that the notice shows up as expected.
We also fix an alignment issue, where the call to refresh the page would be in a distinct column, instead of what we'd expect.
Test plan
Manual testing
Changelog