Fixes #2841 ensures remote node ids are unique

src/commands/remoteProviders.ts

@@ -23,7 +23,7 @@ export class ConnectRemoteProviderCommand extends Command {
 		let args: ConnectRemoteProviderCommandArgs | GitCommit;
 		if (GitRemote.is(argsOrRemote)) {
 			args = {
-				remote: argsOrRemote.id,
+				remote: argsOrRemote.name,
 				repoPath: argsOrRemote.repoPath,
 			};
 		} else {
@@ -39,7 +39,7 @@ export class ConnectRemoteProviderCommand extends Command {
 
 	protected override preExecute(context: CommandContext, args?: ConnectRemoteProviderCommandArgs) {
 		if (isCommandContextViewNodeHasRemote(context)) {
-			args = { ...args, remote: context.node.remote.id, repoPath: context.node.remote.repoPath };
+			args = { ...args, remote: context.node.remote.name, repoPath: context.node.remote.repoPath };
 		}
 
 		return this.execute(args);
@@ -84,7 +84,7 @@ export class ConnectRemoteProviderCommand extends Command {
 			repoPath = args.repoPath;
 
 			remotes = await this.container.git.getRemotesWithProviders(repoPath);
-			remote = remotes.find(r => r.id === args.remote) as GitRemote<RichRemoteProvider> | undefined;
+			remote = remotes.find(r => r.name === args.remote) as GitRemote<RichRemoteProvider> | undefined;
 			if (!remote?.hasRichProvider()) return false;
 		}
 
@@ -112,7 +112,7 @@ export class DisconnectRemoteProviderCommand extends Command {
 		let args: DisconnectRemoteProviderCommandArgs | GitCommit;
 		if (GitRemote.is(argsOrRemote)) {
 			args = {
-				remote: argsOrRemote.id,
+				remote: argsOrRemote.name,
 				repoPath: argsOrRemote.repoPath,
 			};
 		} else {
@@ -131,7 +131,7 @@ export class DisconnectRemoteProviderCommand extends Command {
 
 	protected override preExecute(context: CommandContext, args?: ConnectRemoteProviderCommandArgs) {
 		if (isCommandContextViewNodeHasRemote(context)) {
-			args = { ...args, remote: context.node.remote.id, repoPath: context.node.remote.repoPath };
+			args = { ...args, remote: context.node.remote.name, repoPath: context.node.remote.repoPath };
 		}
 
 		return this.execute(args);
@@ -174,7 +174,7 @@ export class DisconnectRemoteProviderCommand extends Command {
 		} else {
 			repoPath = args.repoPath;
 
-			remote = (await this.container.git.getRemotesWithProviders(repoPath)).find(r => r.id === args.remote) as
+			remote = (await this.container.git.getRemotesWithProviders(repoPath)).find(r => r.name === args.remote) as
 				| GitRemote<RichRemoteProvider>
 				| undefined;
 			if (!remote?.hasRichProvider()) return undefined;

src/git/gitProviderService.ts

@@ -67,7 +67,7 @@ import type { GitRebaseStatus } from './models/rebase';
 import type { GitBranchReference, GitReference } from './models/reference';
 import { createRevisionRange, isSha, isUncommitted, isUncommittedParent } from './models/reference';
 import type { GitReflog } from './models/reflog';
-import { GitRemote } from './models/remote';
+import { getVisibilityCacheKey, GitRemote } from './models/remote';
 import type { RepositoryChangeEvent } from './models/repository';
 import { Repository, RepositoryChange, RepositoryChangeComparisonMode } from './models/repository';
 import type { GitStash } from './models/stash';
@@ -857,15 +857,12 @@ export class GitProviderService implements Disposable {
 		if (visibilityInfo == null) return true;
 
 		if (visibilityInfo.visibility === RepositoryVisibility.Public) {
-			if (remotes.length == 0 || !remotes.some(r => r.id === visibilityInfo.remotesHash)) {
+			if (remotes.length == 0 || !remotes.some(r => r.urlKey === visibilityInfo.remotesHash)) {
 				this.clearRepoVisibilityCache([key]);
 				return false;
 			}
 		} else if (visibilityInfo.visibility === RepositoryVisibility.Private) {
-			const remotesHash = remotes
-				.map(r => r.id)
-				.sort()
-				.join(',');
+			const remotesHash = getVisibilityCacheKey(remotes);
 			if (remotesHash !== visibilityInfo.remotesHash) {
 				this.clearRepoVisibilityCache([key]);
 				return false;

src/git/models/remote.ts

@@ -54,17 +54,8 @@ export class GitRemote
 		);
 	}
 
-	get domain() {
-		return this.provider?.domain ?? this._domain;
-	}
-
-	get path() {
-		return this.provider?.path ?? this._path;
-	}
-
 	constructor(
 		public readonly repoPath: string,
-		public readonly id: string,
 		public readonly name: string,
 		public readonly scheme: string,
 		private readonly _domain: string,
@@ -75,7 +66,23 @@ export class GitRemote
 
 	get default() {
 		const defaultRemote = Container.instance.storage.getWorkspace('remote:default');
-		return this.id === defaultRemote;
+		// Check for `this.urlKey` matches to handle previously saved data
+		return this.name === defaultRemote || this.urlKey === defaultRemote;
+	}
+
+	@memoize()
+	get domain() {
+		return this.provider?.domain ?? this._domain;
+	}
+
+	@memoize()
+	get id() {
+		return `${this.name}/${this.urlKey}`;
+	}
+
+	@memoize()
+	get path() {
+		return this.provider?.path ?? this._path;
 	}
 
 	@memoize()
@@ -94,6 +101,11 @@ export class GitRemote
 		return bestUrl!;
 	}
 
+	@memoize()
+	get urlKey() {
+		return this._domain ? `${this._domain}/${this._path}` : this.path;
+	}
+
 	hasRichProvider(): this is GitRemote<RichRemoteProvider> {
 		return this.provider?.hasRichIntegration() ?? false;
 	}
@@ -175,9 +187,9 @@ export function getRemoteIconUri(
 export function getVisibilityCacheKey(remote: GitRemote): string;
 export function getVisibilityCacheKey(remotes: GitRemote[]): string;
 export function getVisibilityCacheKey(remotes: GitRemote | GitRemote[]): string {
-	if (!Array.isArray(remotes)) return remotes.id;
+	if (!Array.isArray(remotes)) return remotes.urlKey;
 	return remotes
-		.map(r => r.id)
+		.map(r => r.urlKey)
 		.sort()
 		.join(',');
 }

src/git/models/repository.ts

@@ -997,7 +997,7 @@ export class Repository implements Disposable {
 	}
 
 	async setRemoteAsDefault(remote: GitRemote, value: boolean = true) {
-		await this.container.storage.storeWorkspace('remote:default', value ? remote.id : undefined);
+		await this.container.storage.storeWorkspace('remote:default', value ? remote.name : undefined);
 
 		this.fireChange(RepositoryChange.Remotes, RepositoryChange.RemoteProviders);
 	}

src/git/parsers/remoteParser.ts

@@ -47,16 +47,9 @@ export class GitRemoteParser {
 
 			remote = remotes.get(name);
 			if (remote == null) {
-				remote = new GitRemote(
-					repoPath,
-					`${domain ? `${domain}/` : ''}${path}`,
-					name,
-					scheme,
-					domain,
-					path,
-					remoteProviderMatcher(url, domain, path),
-					[{ url: url, type: type as GitRemoteType }],
-				);
+				remote = new GitRemote(repoPath, name, scheme, domain, path, remoteProviderMatcher(url, domain, path), [
+					{ url: url, type: type as GitRemoteType },
+				]);
 				remotes.set(name, remote);
 			} else {
 				remote.urls.push({ url: url, type: type as GitRemoteType });
@@ -65,16 +58,7 @@ export class GitRemoteParser {
 				const provider = remoteProviderMatcher(url, domain, path);
 				if (provider == null) continue;
 
-				remote = new GitRemote(
-					repoPath,
-					`${domain ? `${domain}/` : ''}${path}`,
-					name,
-					scheme,
-					domain,
-					path,
-					provider,
-					remote.urls,
-				);
+				remote = new GitRemote(repoPath, name, scheme, domain, path, provider, remote.urls);
 				remotes.set(name, remote);
 			}
 		} while (true);

src/hovers/hovers.ts

@@ -11,7 +11,7 @@ import { uncommittedStaged } from '../git/models/constants';
 import type { GitDiffHunk, GitDiffHunkLine } from '../git/models/diff';
 import type { PullRequest } from '../git/models/pullRequest';
 import { isUncommittedStaged, shortenRevision } from '../git/models/reference';
-import type { GitRemote } from '../git/models/remote';
+import { GitRemote } from '../git/models/remote';
 import { configuration } from '../system/configuration';
 import { count } from '../system/iterable';
 import { Logger } from '../system/logger';
@@ -223,7 +223,7 @@ export async function detailsMessage(
 			commit.isUncommitted ? commit.getPreviousComparisonUrisForLine(editorLine, uri.sha) : undefined,
 			getAutoLinkedIssuesOrPullRequests(message, remotes),
 			options?.pullRequests?.pr ??
-				getPullRequestForCommit(commit.ref, remotes, {
+				getPullRequestForCommitOrBestRemote(commit.ref, remotes, {
 					pullRequests:
 						options?.pullRequests?.enabled !== false &&
 						CommitFormatter.has(
@@ -246,7 +246,7 @@ export async function detailsMessage(
 	const presence = getSettledValue(presenceResult);
 
 	// Remove possible duplicate pull request
-	if (pr != null && !(pr instanceof PromiseCancelledError)) {
+	if (pr != null && !(pr instanceof PromiseCancelledError || pr instanceof GitRemote)) {
 		autolinkedIssuesOrPullRequests?.delete(pr.id);
 	}
 
@@ -360,7 +360,7 @@ async function getAutoLinkedIssuesOrPullRequests(message: string, remotes: GitRe
 	}
 }
 
-async function getPullRequestForCommit(
+async function getPullRequestForCommitOrBestRemote(
 	ref: string,
 	remotes: GitRemote[],
 	options?: {

src/plus/github/githubGitProvider.ts

@@ -2520,7 +2520,6 @@ export class GitHubGitProvider implements GitProvider, Disposable {
 		return [
 			new GitRemote(
 				repoPath,
-				`${domain}/${path}`,
 				'origin',
 				'https',
 				domain,

src/views/nodes/viewNode.ts

@@ -135,7 +135,7 @@ export function getViewNodeId(type: string, context: AmbientContext): string {
 		uniqueness += `/worktree/${context.worktree.uri.path}`;
 	}
 	if (context.remote != null) {
-		uniqueness += `/remote/${context.remote.id}`;
+		uniqueness += `/remote/${context.remote.name}`;
 	}
 	if (context.tag != null) {
 		uniqueness += `/tag/${context.tag.id}`;