From 2abea6961a3ecb985bfcffb0690b6ebd3f5e63fd Mon Sep 17 00:00:00 2001 From: Sergei Shmakov Date: Sun, 24 Nov 2024 20:05:23 +0100 Subject: [PATCH] Groups repo+prID pairs by provider to perform the more precise search (#3788) --- .../__tests__/pullRequest.utils.test.ts | 61 +++------ src/git/models/pullRequest.ts | 3 +- src/git/models/pullRequest.utils.ts | 27 +--- src/plus/integrations/integration.ts | 3 + .../providers/__tests__/github.utils.test.ts | 80 ++++++++++++ .../providers/__tests__/gitlab.utils.test.ts | 107 ++++++++++++++++ src/plus/integrations/providers/github.ts | 6 + .../integrations/providers/github.utils.ts | 28 +++++ src/plus/integrations/providers/gitlab.ts | 6 + .../integrations/providers/gitlab.utils.ts | 28 +++++ src/plus/launchpad/launchpad.ts | 14 +-- src/plus/launchpad/launchpadProvider.ts | 116 +++++++++++------- 12 files changed, 355 insertions(+), 124 deletions(-) create mode 100644 src/plus/integrations/providers/__tests__/github.utils.test.ts create mode 100644 src/plus/integrations/providers/__tests__/gitlab.utils.test.ts create mode 100644 src/plus/integrations/providers/github.utils.ts create mode 100644 src/plus/integrations/providers/gitlab.utils.ts diff --git a/src/git/models/__tests__/pullRequest.utils.test.ts b/src/git/models/__tests__/pullRequest.utils.test.ts index adeaec4f240b8..59beccf302a4c 100644 --- a/src/git/models/__tests__/pullRequest.utils.test.ts +++ b/src/git/models/__tests__/pullRequest.utils.test.ts @@ -2,61 +2,28 @@ import * as assert from 'assert'; import { suite, test } from 'mocha'; import { getPullRequestIdentityValuesFromSearch } from '../pullRequest.utils'; -suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => { +suite('Test PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => { function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) { assert.deepStrictEqual( getPullRequestIdentityValuesFromSearch(query), - { - ownerAndRepo: ownerAndRepo, - prNumber: prNumber, - }, + prNumber == null + ? undefined + : { + ownerAndRepo: ownerAndRepo, + prNumber: prNumber, + }, `${message} (${JSON.stringify(query)})`, ); } - test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => { - t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens'); - t( - 'with suffix', - 'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello', - '1', - 'eamodio/vscode-gitlens', - ); - t( - 'with query', - 'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello', - '1', - 'eamodio/vscode-gitlens', - ); - - t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens'); - t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens'); - t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1'); - - t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1'); - t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); - }); - - test('no domain, should parse to ownerAndRepo and prNumber', () => { - t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1'); - t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens'); - t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); - }); - - test('domain vs. no domain', () => { - t( - 'with anchor', - 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16', - '1', - 'eamodio/vscode-gitlens', - ); - }); + test('cannot recognize GitHub or GitLab URLs, sees only numbers', () => { + t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16'); + t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '1'); - test('has "pull/" fragment', () => { - t('with leading slash', '/pull/16/files#hello', '16'); - t('without leading slash', 'pull/16?diff=unified#hello', '16'); - t('with numeric repo name', '1/pull/16?diff=unified#hello', '16'); - t('with double slash', '1//pull/16?diff=unified#hello', '16'); + t('no protocol', '/github.com/sergeibbb/1/pull/16?diff=unified', '1'); + t('no domain', '/sergeibbb/1/pull/16#hello', '1'); + t('domain vs. no domain', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/2/pull/16', '1'); + t('has "pull/" fragment', '/pull/16/files#hello', '16'); }); test('has "/" fragment', () => { diff --git a/src/git/models/pullRequest.ts b/src/git/models/pullRequest.ts index 06caee34082fe..7417a80c00707 100644 --- a/src/git/models/pullRequest.ts +++ b/src/git/models/pullRequest.ts @@ -420,11 +420,12 @@ export async function getOpenedPullRequestRepo( export function doesPullRequestSatisfyRepositoryURLIdentity( pr: EnrichablePullRequest | undefined, - { ownerAndRepo, prNumber }: PullRequestURLIdentity, + prUrlIdentity: { [key in string]?: PullRequestURLIdentity }, ): boolean { if (pr == null) { return false; } + const { ownerAndRepo, prNumber } = prUrlIdentity[pr.provider.id] ?? {}; const satisfiesPrNumber = prNumber != null && pr.number === parseInt(prNumber, 10); if (!satisfiesPrNumber) { return false; diff --git a/src/git/models/pullRequest.utils.ts b/src/git/models/pullRequest.utils.ts index 57df8970a25de..63414f8a040fc 100644 --- a/src/git/models/pullRequest.utils.ts +++ b/src/git/models/pullRequest.utils.ts @@ -4,31 +4,16 @@ export type PullRequestURLIdentity = { ownerAndRepo?: string; - prNumber?: string; + prNumber: string; }; -export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestURLIdentity { - let ownerAndRepo: string | undefined = undefined; +export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestURLIdentity | undefined { + const ownerAndRepo: string | undefined = undefined; let prNumber: string | undefined = undefined; - let match = search.match(/([^/]+\/[^/]+)\/(?:pull|-\/merge_requests)\/(\d+)/); // with org and rep name + let match = search.match(/(?:\/)(\d+)/); // any number starting with "/" if (match != null) { - ownerAndRepo = match[1]; - prNumber = match[2]; - } - - if (prNumber == null) { - match = search.match(/(?:\/|^)(?:pull|-\/merge_requests)\/(\d+)/); // without repo name - if (match != null) { - prNumber = match[1]; - } - } - - if (prNumber == null) { - match = search.match(/(?:\/)(\d+)/); // any number starting with "/" - if (match != null) { - prNumber = match[1]; - } + prNumber = match[1]; } if (prNumber == null) { @@ -38,5 +23,5 @@ export function getPullRequestIdentityValuesFromSearch(search: string): PullRequ } } - return { ownerAndRepo: ownerAndRepo, prNumber: prNumber }; + return prNumber == null ? undefined : { ownerAndRepo: ownerAndRepo, prNumber: prNumber }; } diff --git a/src/plus/integrations/integration.ts b/src/plus/integrations/integration.ts index 9e8b362b433d6..8d6fa1ac8975c 100644 --- a/src/plus/integrations/integration.ts +++ b/src/plus/integrations/integration.ts @@ -16,6 +16,7 @@ import type { PullRequestState, SearchedPullRequest, } from '../../git/models/pullRequest'; +import type { PullRequestURLIdentity } from '../../git/models/pullRequest.utils'; import type { RepositoryMetadata } from '../../git/models/repositoryMetadata'; import { showIntegrationDisconnectedTooManyFailedRequestsWarningMessage } from '../../messages'; import { gate } from '../../system/decorators/gate'; @@ -1292,4 +1293,6 @@ export abstract class HostingIntegration< repos?: T[], cancellation?: CancellationToken, ): Promise; + + getPullRequestIdentityValuesFromSearch?(search: string): PullRequestURLIdentity | undefined; } diff --git a/src/plus/integrations/providers/__tests__/github.utils.test.ts b/src/plus/integrations/providers/__tests__/github.utils.test.ts new file mode 100644 index 0000000000000..2042148886229 --- /dev/null +++ b/src/plus/integrations/providers/__tests__/github.utils.test.ts @@ -0,0 +1,80 @@ +import * as assert from 'assert'; +import { suite, test } from 'mocha'; +import { getGitHubPullRequestIdentityValuesFromSearch } from '../github.utils'; + +suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => { + function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) { + assert.deepStrictEqual( + getGitHubPullRequestIdentityValuesFromSearch(query), + prNumber == null + ? undefined + : { + ownerAndRepo: ownerAndRepo, + prNumber: prNumber, + }, + `${message} (${JSON.stringify(query)})`, + ); + } + + test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => { + t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens'); + t( + 'with suffix', + 'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t( + 'with query', + 'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + + t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens'); + t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens'); + t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1'); + + t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1'); + t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); + }); + + test('no domain, should parse to ownerAndRepo and prNumber', () => { + t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1'); + t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens'); + t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); + }); + + test('domain vs. no domain', () => { + t( + 'with anchor', + 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16', + '1', + 'eamodio/vscode-gitlens', + ); + }); + + test('numbers', () => { + t('has "pull/" fragment', '/pull/16/files#hello', '16'); + t('has "pull/" fragment with double slash', '1//pull/16?diff=unified#hello', '16'); + t('with leading slash', '/16/files#hello', '16'); + t('just a number', '16', '16'); + t('with a hash', '#16', '16'); + }); + + test('does not match', () => { + t('without leading slash', '16?diff=unified#hello', undefined); + t('with leading hash', '/#16/files#hello', undefined); + t('number is a part of a word', 'hello16', undefined); + t('number is a part of a word', '16hello', undefined); + + t('GitLab', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/16', '16'); + + t('with a number', '1/16?diff=unified#hello', '16'); + t('with a number and slash', '/1/16?diff=unified#hello', '1'); + t('with a word', 'anything/16?diff=unified#hello', '16'); + + t('with a wrong character leading to pull', 'sergeibbb/1/-pull/16?diff=unified#hello', '1'); + t('with a wrong character leading to pull', 'sergeibbb/1-pull/16?diff=unified#hello', '1'); + }); +}); diff --git a/src/plus/integrations/providers/__tests__/gitlab.utils.test.ts b/src/plus/integrations/providers/__tests__/gitlab.utils.test.ts new file mode 100644 index 0000000000000..81cbca0ab92c1 --- /dev/null +++ b/src/plus/integrations/providers/__tests__/gitlab.utils.test.ts @@ -0,0 +1,107 @@ +import * as assert from 'assert'; +import { suite, test } from 'mocha'; +import { getGitLabPullRequestIdentityValuesFromSearch } from '../gitlab.utils'; + +suite('Test GitLab PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => { + function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) { + assert.deepStrictEqual( + getGitLabPullRequestIdentityValuesFromSearch(query), + prNumber == null + ? undefined + : { + ownerAndRepo: ownerAndRepo, + prNumber: prNumber, + }, + `${message} (${JSON.stringify(query)})`, + ); + } + + test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => { + t('full URL', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1', '1', 'eamodio/vscode-gitlens'); + t( + 'with suffix', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1/files?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t( + 'with query', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + + t( + 'with anchor', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t( + 'a weird suffix', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1-files', + '1', + 'eamodio/vscode-gitlens', + ); + t('numeric repo name', 'https://gitlab.com/sergeibbb/1/-/merge_requests/16', '16', 'sergeibbb/1'); + + t( + 'no protocol with leading slash', + '/gitlab.com/sergeibbb/1/-/merge_requests/16?diff=unified', + '16', + 'sergeibbb/1', + ); + t('no protocol without leading slash', 'gitlab.com/sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1'); + }); + test('no domain, should parse to ownerAndRepo and prNumber', () => { + t('with leading slash', '/sergeibbb/1/-/merge_requests/16#hello', '16', 'sergeibbb/1'); + t( + 'words in repo name', + 'eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t('numeric repo name', 'sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1'); + }); + + test('domain vs. no domain', () => { + t( + 'with anchor', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello/sergeibbb/1/-/merge_requests/16', + '1', + 'eamodio/vscode-gitlens', + ); + }); + + test('numbers', () => { + t('has "-/merge_requests/" fragment', '/-/merge_requests/16/files#hello', '16'); + t('has "-/merge_requests/" fragment with double slash', '1//-/merge_requests/16?diff=unified#hello', '16'); + t('with leading slash', '/16/files#hello', '16'); + t('just a number', '16', '16'); + t('with a hash', '#16', '16'); + }); + + test('does not match', () => { + t('without leading slash', '16?diff=unified#hello', undefined); + t('with leading hash', '/#16/files#hello', undefined); + t('number is a part of a word', 'hello16', undefined); + t('number is a part of a word', '16hello', undefined); + + t('GitHub', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16'); + + t('with a number', '1/16?diff=unified#hello', '16'); + t('with a number and slash', '/1/16?diff=unified#hello', '1'); + t('with a word', 'anything/16?diff=unified#hello', '16'); + + t( + 'with a wrong character leading to "-/merge_requests/"', + 'sergeibbb/1/--/merge_requests/16?diff=unified#hello', + '1', + ); + t( + 'with a wrong character leading to "-/merge_requests/"', + 'sergeibbb/1--/merge_requests/16?diff=unified#hello', + '1', + ); + }); +}); diff --git a/src/plus/integrations/providers/github.ts b/src/plus/integrations/providers/github.ts index 4145d3617b4c2..13848b48600ad 100644 --- a/src/plus/integrations/providers/github.ts +++ b/src/plus/integrations/providers/github.ts @@ -11,6 +11,7 @@ import type { PullRequestState, SearchedPullRequest, } from '../../../git/models/pullRequest'; +import type { PullRequestURLIdentity } from '../../../git/models/pullRequest.utils'; import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata'; import { log } from '../../../system/decorators/log'; import { ensurePaidPlan } from '../../utils'; @@ -20,6 +21,7 @@ import type { } from '../authentication/integrationAuthentication'; import type { SupportedIntegrationIds } from '../integration'; import { HostingIntegration } from '../integration'; +import { getGitHubPullRequestIdentityValuesFromSearch } from './github.utils'; import { providersMetadata } from './models'; import type { ProvidersApi } from './providersApi'; @@ -285,6 +287,10 @@ export class GitHubIntegration extends GitHubIntegrationBase { diff --git a/src/plus/integrations/providers/github.utils.ts b/src/plus/integrations/providers/github.utils.ts new file mode 100644 index 0000000000000..851d7e3fdaefd --- /dev/null +++ b/src/plus/integrations/providers/github.utils.ts @@ -0,0 +1,28 @@ +// GitHub provider: github.ts pulls many dependencies through Container and some of them break the unit tests. +// That's why this file has been created that can collect more simple functions which +// don't require Container and can be tested. + +import type { PullRequestURLIdentity } from '../../../git/models/pullRequest.utils'; +import { getPullRequestIdentityValuesFromSearch } from '../../../git/models/pullRequest.utils'; + +export function getGitHubPullRequestIdentityValuesFromSearch(search: string): PullRequestURLIdentity | undefined { + let ownerAndRepo: string | undefined = undefined; + let prNumber: string | undefined = undefined; + + let match = search.match(/([^/]+\/[^/]+)\/(?:pull)\/(\d+)/); // with org and rep name + if (match != null) { + ownerAndRepo = match[1]; + prNumber = match[2]; + } + + if (prNumber == null) { + match = search.match(/(?:\/|^)(?:pull)\/(\d+)/); // without repo name + if (match != null) { + prNumber = match[1]; + } + } + + return prNumber != null + ? { ownerAndRepo: ownerAndRepo, prNumber: prNumber } + : getPullRequestIdentityValuesFromSearch(search); +} diff --git a/src/plus/integrations/providers/gitlab.ts b/src/plus/integrations/providers/gitlab.ts index c701e43b955cb..b03b891290076 100644 --- a/src/plus/integrations/providers/gitlab.ts +++ b/src/plus/integrations/providers/gitlab.ts @@ -12,6 +12,7 @@ import type { PullRequestState, SearchedPullRequest, } from '../../../git/models/pullRequest'; +import type { PullRequestURLIdentity } from '../../../git/models/pullRequest.utils'; import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata'; import { log } from '../../../system/decorators/log'; import { uniqueBy } from '../../../system/iterable'; @@ -22,6 +23,7 @@ import type { } from '../authentication/integrationAuthentication'; import { HostingIntegration } from '../integration'; import { fromGitLabMergeRequestProvidersApi } from './gitlab/models'; +import { getGitLabPullRequestIdentityValuesFromSearch } from './gitlab.utils'; import type { ProviderPullRequest } from './models'; import { ProviderPullRequestReviewState, providersMetadata } from './models'; import type { ProvidersApi } from './providersApi'; @@ -331,6 +333,10 @@ abstract class GitLabIntegrationBase< username: currentUser.username || undefined, }; } + + override getPullRequestIdentityValuesFromSearch(search: string): PullRequestURLIdentity | undefined { + return getGitLabPullRequestIdentityValuesFromSearch(search); + } } export class GitLabIntegration extends GitLabIntegrationBase { diff --git a/src/plus/integrations/providers/gitlab.utils.ts b/src/plus/integrations/providers/gitlab.utils.ts new file mode 100644 index 0000000000000..b142bf66c87f4 --- /dev/null +++ b/src/plus/integrations/providers/gitlab.utils.ts @@ -0,0 +1,28 @@ +// GitLab provider: gitlab.ts pulls many dependencies through Container and some of them break the unit tests. +// That's why this file has been created that can collect more simple functions which +// don't require Container and can be tested. + +import type { PullRequestURLIdentity } from '../../../git/models/pullRequest.utils'; +import { getPullRequestIdentityValuesFromSearch } from '../../../git/models/pullRequest.utils'; + +export function getGitLabPullRequestIdentityValuesFromSearch(search: string): PullRequestURLIdentity | undefined { + let ownerAndRepo: string | undefined = undefined; + let prNumber: string | undefined = undefined; + + let match = search.match(/([^/]+\/[^/]+)\/(?:-\/merge_requests)\/(\d+)/); // with org and rep name + if (match != null) { + ownerAndRepo = match[1]; + prNumber = match[2]; + } + + if (prNumber == null) { + match = search.match(/(?:\/|^)(?:-\/merge_requests)\/(\d+)/); // without repo name + if (match != null) { + prNumber = match[1]; + } + } + + return prNumber != null + ? { ownerAndRepo: ownerAndRepo, prNumber: prNumber } + : getPullRequestIdentityValuesFromSearch(search); +} diff --git a/src/plus/launchpad/launchpad.ts b/src/plus/launchpad/launchpad.ts index 1e9fac390f219..6fef5149e2849 100644 --- a/src/plus/launchpad/launchpad.ts +++ b/src/plus/launchpad/launchpad.ts @@ -43,7 +43,7 @@ import type { LaunchpadTelemetryContext, Source, Sources, TelemetryEvents } from import type { Container } from '../../container'; import { PlusFeatures } from '../../features'; import { doesPullRequestSatisfyRepositoryURLIdentity } from '../../git/models/pullRequest'; -import { getPullRequestIdentityValuesFromSearch } from '../../git/models/pullRequest.utils'; +import type { PullRequestURLIdentity } from '../../git/models/pullRequest.utils'; import type { QuickPickItemOfT } from '../../quickpicks/items/common'; import { createQuickPickItemOfT, createQuickPickSeparator } from '../../quickpicks/items/common'; import type { DirectiveQuickPickItem } from '../../quickpicks/items/directive'; @@ -599,14 +599,10 @@ export class LaunchpadCommand extends QuickCommand { return true; } - // TODO: This needs to be generalized to work outside of GitHub, - // The current idea is that we should iterate the connected integrations and apply their parsing. - // Probably we even want to build a map like this: { integrationId: identity } - // Then when we iterate local items we can check them to corresponding identitie according to the item's repo type. - // Same with API: we iterate connected integrations and search in each of them with the corresponding identity. - const prUrlIdentity = getPullRequestIdentityValuesFromSearch(value); + const prUrlIdentity: { [key in string]?: PullRequestURLIdentity } = + await this.container.launchpad.getPullRequestIdentityValuesFromSearch(value); - if (prUrlIdentity.prNumber != null) { + if (Object.keys(prUrlIdentity).length > 0) { // We can identify the PR number, so let's try to find it locally: const launchpadItems = quickpick.items.filter((i): i is LaunchpadItemQuickPickItem => 'item' in i); let item = launchpadItems.find(i => @@ -615,7 +611,7 @@ export class LaunchpadCommand extends QuickCommand { ); if (item == null) { // Haven't found full match, so let's at least find something with the same pr number - item = launchpadItems.find(i => i.item.id === prUrlIdentity.prNumber); + item = launchpadItems.find(i => i.item.id === prUrlIdentity[i.item.provider.id]?.prNumber); } if (item != null) { if (!item.alwaysShow) { diff --git a/src/plus/launchpad/launchpadProvider.ts b/src/plus/launchpad/launchpadProvider.ts index 3535ffb59390c..33ad5751813fc 100644 --- a/src/plus/launchpad/launchpadProvider.ts +++ b/src/plus/launchpad/launchpadProvider.ts @@ -22,7 +22,6 @@ import { getRepositoryIdentityForPullRequest, } from '../../git/models/pullRequest'; import type { PullRequestURLIdentity } from '../../git/models/pullRequest.utils'; -import { getPullRequestIdentityValuesFromSearch } from '../../git/models/pullRequest.utils'; import type { GitRemote } from '../../git/models/remote'; import type { Repository } from '../../git/models/repository'; import type { CodeSuggestionCounts, Draft } from '../../gk/models/drafts'; @@ -327,7 +326,8 @@ export class LaunchpadProvider implements Disposable { // The current idea is that we should iterate the connected integrations and apply their parsing. // Probably we even want to build a map like this: { integrationId: identity } // Then we iterate connected integrations and search in each of them with the corresponding identity. - const prUrlIdentity = getPullRequestIdentityValuesFromSearch(search); + const prUrlIdentity: { [key in IntegrationId]?: PullRequestURLIdentity } = + await this.getPullRequestIdentityValuesFromSearch(search); const result: { readonly value: SearchedPullRequest[]; duration: number } = { value: [], duration: 0, @@ -335,20 +335,55 @@ export class LaunchpadProvider implements Disposable { const connectedIntegrations = await this.getConnectedIntegrations(); + const findByUrlIdentity = async ( + integration: HostingIntegration, + ): Promise> => { + const { ownerAndRepo, prNumber } = prUrlIdentity[integration.id] ?? {}; + if (prNumber != null && ownerAndRepo != null) { + const [owner, repo] = ownerAndRepo.split('/', 2); + const descriptor: GitHubRepositoryDescriptor = { + key: ownerAndRepo, + owner: owner, + name: repo, + }; + const pr = await withDurationAndSlowEventOnTimeout( + integration?.getPullRequest(descriptor, prNumber), + 'getPullRequest', + this.container, + ); + if (pr?.value != null) { + return { value: [{ pullRequest: pr.value, reasons: [] }], duration: pr.duration }; + } + } + return undefined; + }; + + const findByQuery = async ( + integration: HostingIntegration, + ): Promise> => { + const prs = await withDurationAndSlowEventOnTimeout( + integration?.searchPullRequests(search, undefined, cancellation), // + 'searchPullRequests', + this.container, + ); + if (prs != null) { + return { value: prs.value?.map(pr => ({ pullRequest: pr, reasons: [] })), duration: prs.duration }; + } + return undefined; + }; + + const hasUrlIdentity = Object.keys(prUrlIdentity).length > 0; + const searchIntegrationPRs = hasUrlIdentity ? findByUrlIdentity : findByQuery; + await Promise.allSettled( [...connectedIntegrations.keys()] .filter( (id: IntegrationId): id is SupportedLaunchpadIntegrationIds => (connectedIntegrations.get(id) && isSupportedLaunchpadIntegrationId(id)) ?? false, ) - .map(async (id: HostingIntegrationId) => { + .map(async (id: SupportedLaunchpadIntegrationIds) => { const integration = await this.container.integrations.get(id); - const searchResult = await this.searchIntegrationPRs( - search, - prUrlIdentity, - integration, - cancellation, - ); + const searchResult = await searchIntegrationPRs(integration); const prs = searchResult?.value; if (prs) { result.value?.push(...prs); @@ -362,43 +397,6 @@ export class LaunchpadProvider implements Disposable { }; } - private async searchIntegrationPRs( - search: string, - { ownerAndRepo, prNumber }: PullRequestURLIdentity, - integration: HostingIntegration, - cancellation: CancellationToken | undefined, - ): Promise> { - let result: TimedResult | undefined; - if (prNumber != null && ownerAndRepo != null) { - const [owner, repo] = ownerAndRepo.split('/', 2); - const descriptor: GitHubRepositoryDescriptor = { - key: ownerAndRepo, - owner: owner, - name: repo, - }; - const pr = await withDurationAndSlowEventOnTimeout( - integration?.getPullRequest(descriptor, prNumber), - 'getPullRequest', - this.container, - ); - if (pr?.value != null) { - result = { value: [{ pullRequest: pr.value, reasons: [] }], duration: pr.duration }; - return result; - } - } else { - const prs = await withDurationAndSlowEventOnTimeout( - integration?.searchPullRequests(search, undefined, cancellation), // - 'searchPullRequests', - this.container, - ); - if (prs != null) { - result = { value: prs.value?.map(pr => ({ pullRequest: pr, reasons: [] })), duration: prs.duration }; - return result; - } - } - return undefined; - } - private _enrichedItems: CachedLaunchpadPromise> | undefined; @debug({ args: { 0: o => `force=${o?.force}` } }) private async getEnrichedItems(options?: { cancellation?: CancellationToken; force?: boolean }) { @@ -696,6 +694,32 @@ export class LaunchpadProvider implements Disposable { return repoRemotes; } + async getPullRequestIdentityValuesFromSearch(search: string): Promise<{ + [key in SupportedLaunchpadIntegrationIds]?: PullRequestURLIdentity; + }> { + const result: Partial> = {}; + const fullResult: Partial> = {}; + let hasFullResult = false; + + const connectedIntegrations = await this.getConnectedIntegrations(); + + for (const integrationId of supportedLaunchpadIntegrations) { + if (connectedIntegrations.get(integrationId)) { + const integration = await this.container.integrations.get(integrationId); + const prIdentity = integration.getPullRequestIdentityValuesFromSearch?.(search); + if (prIdentity) { + result[integrationId] = prIdentity; + } + if (prIdentity?.ownerAndRepo != null) { + fullResult[integrationId] = prIdentity; + hasFullResult = true; + } + } + } + + return hasFullResult ? fullResult : result; + } + @gate(o => `${o?.force ?? false}`) @log({ args: { 0: o => `force=${o?.force}`, 1: false } }) async getCategorizedItems(