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

Improves overview branch autolinks #3957

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/autolinks/autolinks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export class Autolinks implements Disposable {
linkIntegration = undefined;
}
}
const issueOrPullRequestPromise =
let issueOrPullRequestPromise =
remote?.provider != null &&
integration != null &&
link.provider?.id === integration.id &&
Expand All @@ -235,6 +235,13 @@ export class Autolinks implements Disposable {
: link.descriptor != null
? linkIntegration?.getIssueOrPullRequest(link.descriptor, this.getAutolinkEnrichableId(link))
: undefined;
// we consider that all non-prefixed links are came from branch names and linked to issues
// skip if it's a PR link
if (!link.prefix) {
issueOrPullRequestPromise = issueOrPullRequestPromise?.then(x =>
x?.type === 'pullrequest' ? undefined : x,
);
}
Comment on lines +238 to +244
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what "non-prefixed" means a bit so I can understand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

non-prefixed refset is required to parse branch name autolinks and mean that we're trying to search issue keys in any numbers with no prefixes. For example, we can search issues with prefixes # or gh for github in commit messages, but for the branch names the issue keys can be non-prefixed - just numbers. like in current branch name
bugs/3956-pr-autolinks-are-rendered-in-issues-section - the issue key is just a number without any prefixes

enrichedAutolinks.set(id, [issueOrPullRequestPromise, link]);
}

Expand Down
1 change: 1 addition & 0 deletions src/constants.commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ export type TreeViewCommandSuffixesByViewType<T extends TreeViewTypes> = Extract
>;

type HomeWebviewCommands = `home.${
| 'unlinkIssue'
| 'openMergeTargetComparison'
| 'openPullRequestChanges'
| 'openPullRequestComparison'
Expand Down
4 changes: 4 additions & 0 deletions src/constants.storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ export type GlobalStorage = {
'graph:searchMode': StoredGraphSearchMode;
'views:scm:grouped:welcome:dismissed': boolean;
'integrations:configured': StoredIntegrationConfigurations;
'autolinks:branches:ignore': IgnoredBranchesAutolinks;
'autolinks:branches:ignore:skipPrompt': boolean | undefined;
} & { [key in `plus:preview:${FeaturePreviews}:usages`]: StoredFeaturePreviewUsagePeriod[] } & {
[key in `confirm:ai:tos:${AIProviders}`]: boolean;
} & {
Expand All @@ -95,6 +97,8 @@ export type GlobalStorage = {

export type StoredIntegrationConfigurations = Record<string, StoredConfiguredIntegrationDescriptor[] | undefined>;

export type IgnoredBranchesAutolinks = Record<string, string[] | undefined>;

export interface StoredConfiguredIntegrationDescriptor {
cloud: boolean;
integrationId: IntegrationId;
Expand Down
11 changes: 10 additions & 1 deletion src/git/models/branch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { Container } from '../../container';
import { formatDate, fromNow } from '../../system/date';
import { memoize } from '../../system/decorators/-webview/memoize';
import { debug } from '../../system/decorators/log';
import { forEach } from '../../system/iterable';
import { getLoggableName } from '../../system/logger';
import type { MaybePausedResult } from '../../system/promise';
import {
Expand Down Expand Up @@ -118,9 +119,17 @@ export class GitBranch implements GitBranchReference {
}

@memoize()
async getEnrichedAutolinks(): Promise<Map<string, EnrichedAutolink> | undefined> {
async getEnrichedAutolinks(ignoredLinks?: string[]): Promise<Map<string, EnrichedAutolink> | undefined> {
const remote = await this.container.git.remotes(this.repoPath).getBestRemoteWithProvider();
const branchAutolinks = await this.container.autolinks.getBranchAutolinks(this.name, remote);
if (ignoredLinks?.length) {
const ignoredMap = Object.fromEntries(ignoredLinks.map(x => [x, true]));
forEach(branchAutolinks, ([key, link]) => {
if (ignoredMap[link.url]) {
branchAutolinks.delete(key);
}
});
}
return this.container.autolinks.getEnrichedAutolinks(branchAutolinks, remote);
}

Expand Down
61 changes: 54 additions & 7 deletions src/webviews/apps/plus/home/components/branch-card.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import type { AssociateIssueWithBranchCommandArgs } from '../../../../../plus/st
import { createCommandLink } from '../../../../../system/commands';
import { fromNow } from '../../../../../system/date';
import { interpolate, pluralize } from '../../../../../system/string';
import type { BranchRef, GetOverviewBranch, OpenInGraphParams } from '../../../../home/protocol';
import type { BranchIssueLink, BranchRef, GetOverviewBranch, OpenInGraphParams } from '../../../../home/protocol';
import { renderBranchName } from '../../../shared/components/branch-name';
import type { GlCard } from '../../../shared/components/card/card';
import { GlElement, observe } from '../../../shared/components/element';
Expand Down Expand Up @@ -58,6 +58,25 @@ export const branchCardStyles = css`
flex-direction: column;
gap: 0.4rem;
}

.branch-item__unplug {
padding: 0.2em;
margin-block: -0.2em;
opacity: 0;
border-radius: 3px;
}

.branch-item__section:hover .branch-item__unplug,
.branch-item__section:focus-within .branch-item__unplug {
opacity: 1;
}

.branch-item__unplug:hover,
.branch-item__unplug:focus {
background-color: var(--vscode-toolbar-hoverBackground);
outline: 1px dashed var(--vscode-toolbar-hoverOutline);
}

.branch-item__section > * {
margin-block: 0;
}
Expand Down Expand Up @@ -499,19 +518,47 @@ export abstract class GlBranchCardBase extends GlElement {
this.toggleExpanded(true);
}

protected renderIssues(): TemplateResult | NothingType {
private getIssues(): BranchIssueLink[] {
const { autolinks, issues } = this;
const issuesSource = issues?.length ? issues : autolinks;
if (!issuesSource?.length) return nothing;
const issuesMap: Record<string, BranchIssueLink> = {};
autolinks?.map(autolink => {
if (autolink.type !== 'issue') {
return;
}
issuesMap[autolink.url] = autolink;
});
issues?.map(issue => {
issuesMap[issue.url] = issue;
});
return Object.values(issuesMap);
}

protected renderIssues(issues: BranchIssueLink[]) {
if (!issues.length) return nothing;
return html`
${issuesSource.map(issue => {
${issues.map(issue => {
return html`
<p class="branch-item__grouping">
<span class="branch-item__icon">
<issue-icon state=${issue.state} issue-id=${issue.id}></issue-icon>
</span>
<a href=${issue.url} class="branch-item__name branch-item__name--secondary">${issue.title}</a>
${when(
issue.isAutolink && this.expanded,
() => html`
<gl-tooltip>
<a
class="branch-item__unplug"
href=${createCommandLink('gitlens.home.unlinkIssue', {
issue: issue,
reference: this.branch.reference,
})}
><code-icon icon="gl-unplug"></code-icon
></a>
<div slot="content">Unlink automatically linked issue</div>
</gl-tooltip>
`,
)}
<span class="branch-item__identifier">#${issue.id}</span>
</p>
`;
Expand Down Expand Up @@ -791,7 +838,7 @@ export abstract class GlBranchCardBase extends GlElement {
}

protected renderIssuesItem(): TemplateResult | NothingType {
const issues = [...(this.issues ?? []), ...(this.autolinks ?? [])];
const issues = this.getIssues();
if (!issues.length) {
if (!this.expanded) return nothing;

Expand Down Expand Up @@ -821,7 +868,7 @@ export abstract class GlBranchCardBase extends GlElement {

return html`
<gl-work-item ?expanded=${this.expanded} ?nested=${!this.branch.opened} .indicator=${indicator}>
<div class="branch-item__section">${this.renderIssues()}</div>
<div class="branch-item__section">${this.renderIssues(issues)}</div>
</gl-work-item>
`;
}
Expand Down
37 changes: 36 additions & 1 deletion src/webviews/home/homeWebview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import type { GitFileChangeShape } from '../../git/models/fileChange';
import type { Issue } from '../../git/models/issue';
import type { GitPausedOperationStatus } from '../../git/models/pausedOperationStatus';
import type { PullRequest } from '../../git/models/pullRequest';
import type { GitBranchReference } from '../../git/models/reference';
import { RemoteResourceType } from '../../git/models/remoteResource';
import type { Repository, RepositoryFileSystemChangeEvent } from '../../git/models/repository';
import { RepositoryChange, RepositoryChangeComparisonMode } from '../../git/models/repository';
Expand Down Expand Up @@ -67,6 +68,7 @@ import type { IpcMessage } from '../protocol';
import type { WebviewHost, WebviewProvider, WebviewShowingArgs } from '../webviewProvider';
import type { WebviewShowOptions } from '../webviewsController';
import type {
BranchIssueLink,
BranchRef,
CollapseSectionParams,
DidChangeRepositoriesParams,
Expand Down Expand Up @@ -332,6 +334,7 @@ export class HomeWebviewProvider implements WebviewProvider<State, State, HomeWe
registerCommand('gitlens.home.continuePausedOperation', this.continuePausedOperation, this),
registerCommand('gitlens.home.abortPausedOperation', this.abortPausedOperation, this),
registerCommand('gitlens.home.openRebaseEditor', this.openRebaseEditor, this),
registerCommand('gitlens.home.unlinkIssue', this.unlinkIssue, this),
];
}

Expand Down Expand Up @@ -545,6 +548,35 @@ export class HomeWebviewProvider implements WebviewProvider<State, State, HomeWe
});
}

private async unlinkIssue({ issue, reference }: { reference: GitBranchReference; issue: BranchIssueLink }) {
const skipPrompt = this.container.storage.get('autolinks:branches:ignore:skipPrompt') || undefined;
const item =
skipPrompt ??
(await window.showWarningMessage(
`This action will unlink the issue ${issue.url} from the branch ${reference.name} forever`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of something that cannot be undone like this. I think we need a broader discussion on how we want to handle issue association and dissociation and how autolinks fit into that mix, in a way that prevents anyone from getting stuck into a state they cannot easily back out from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be discussed on today's daily standup. @justinrobots

{
modal: true,
},
`OK`,
`OK, Don't ask again`,
));
if (!item) {
return;
}
if (item === `OK, Don't ask again`) {
void this.container.storage.store('autolinks:branches:ignore:skipPrompt', true);
}
const prev = this.container.storage.get('autolinks:branches:ignore') ?? {};
const refId = reference.id ?? `${reference.repoPath}/${reference.remote}/${reference.ref}`;
await this.container.storage
.store('autolinks:branches:ignore', {
...prev,
[refId]: [...(prev[refId] ?? []), issue.url],
})
.catch();
void this.host.notify(DidChangeRepositoryWip, undefined);
}

private async createCloudPatch(ref: BranchRef) {
const status = await this.container.git.status(ref.repoPath).getStatus();
if (status == null) return;
Expand Down Expand Up @@ -1357,11 +1389,12 @@ function getOverviewBranchesCore(
for (const branch of branches) {
const wt = worktreesByBranch.get(branch.id);

const ignored = container.storage.get('autolinks:branches:ignore')?.[branch.id];
const timestamp = branch.date?.getTime();

if (isPro === true) {
prPromises.set(branch.id, getPullRequestInfo(container, branch, launchpadPromise));
autolinkPromises.set(branch.id, branch.getEnrichedAutolinks());
autolinkPromises.set(branch.id, branch.getEnrichedAutolinks(ignored));
issuePromises.set(
branch.id,
getAssociatedIssuesForBranch(container, branch).then(issues => issues.value),
Expand Down Expand Up @@ -1472,6 +1505,8 @@ async function getAutolinkIssuesInfo(links: Map<string, EnrichedAutolink> | unde
title: issue.title,
url: issue.url,
state: issue.state,
type: issue.type,
isAutolink: true,
};
}),
);
Expand Down
27 changes: 11 additions & 16 deletions src/webviews/home/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { IntegrationDescriptor } from '../../constants.integrations';
import type { GitBranchMergedStatus } from '../../git/gitProvider';
import type { GitBranchStatus, GitTrackingState } from '../../git/models/branch';
import type { Issue } from '../../git/models/issue';
import type { IssueOrPullRequestType } from '../../git/models/issueOrPullRequest';
import type { MergeConflict } from '../../git/models/mergeConflict';
import type { GitPausedOperationStatus } from '../../git/models/pausedOperationStatus';
import type { GitBranchReference } from '../../git/models/reference';
Expand Down Expand Up @@ -62,6 +63,14 @@ export const GetLaunchpadSummary = new IpcRequest<GetLaunchpadSummaryRequest, Ge
'launchpad/summary',
);

export interface BranchIssueLink {
id: string;
title: string;
url: string;
state: Omit<Issue['state'], 'merged'>;
isAutolink?: boolean;
}

export interface GetOverviewBranch {
reference: GitBranchReference;

Expand Down Expand Up @@ -161,23 +170,9 @@ export interface GetOverviewBranch {
| undefined
>;

autolinks?: Promise<
{
id: string;
title: string;
url: string;
state: Omit<Issue['state'], 'merged'>;
}[]
>;
autolinks?: Promise<(BranchIssueLink & { type: IssueOrPullRequestType })[]>;

issues?: Promise<
{
id: string;
title: string;
url: string;
state: Omit<Issue['state'], 'merged'>;
}[]
>;
issues?: Promise<BranchIssueLink[]>;

worktree?: {
name: string;
Expand Down