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

Enhance Branch Creation UX and Refactor Issue Branch Management #6278

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e4f0d7a
refactor(issue-branch): enhance branch selection and creation workflow
amrshadid Oct 13, 2024
6808386
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 16, 2024
b59e5a9
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 16, 2024
aff10c4
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 17, 2024
fb62463
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 17, 2024
409a968
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 18, 2024
30a9a0c
fix: Ensure proper handling of ISSUE_BRANCH_TITLE and silent flag
amrshadid Oct 19, 2024
cefb91f
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 21, 2024
212c6e0
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 22, 2024
1c807cc
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 23, 2024
ba091f0
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 23, 2024
96d5658
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 24, 2024
b25d5f8
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 25, 2024
139b258
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 28, 2024
41f1c32
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 29, 2024
e3cf4dc
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 29, 2024
13ce6b0
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 30, 2024
e4f989d
Merge branch 'main' into feature/branch-search-improvements
amrshadid Oct 31, 2024
c1eb15d
Merge branch 'main' into feature/branch-search-improvements
amrshadid Nov 6, 2024
44903c2
Merge branch 'main' into feature/branch-search-improvements
amrshadid Nov 7, 2024
2844546
Merge branch 'main' into feature/branch-search-improvements
amrshadid Nov 13, 2024
7aba54b
Merge branch 'main' into feature/branch-search-improvements
amrshadid Nov 23, 2024
a16f2ee
Merge branch 'main' into feature/branch-search-improvements
amrshadid Dec 7, 2024
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
230 changes: 146 additions & 84 deletions src/issues/currentIssue.ts
alexr00 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
WORKING_ISSUE_FORMAT_SCM,
} from '../common/settingKeys';
import { FolderRepositoryManager, PullRequestDefaults } from '../github/folderRepositoryManager';
import { GithubItemStateEnum } from '../github/interface';
import { IssueModel } from '../github/issueModel';
import { variableSubstitution } from '../github/utils';
import { IssueState, StateManager } from './stateManager';
Expand All @@ -28,6 +27,7 @@ export class CurrentIssue {
private _repoDefaults: PullRequestDefaults | undefined;
private _onDidChangeCurrentIssueState: vscode.EventEmitter<void> = new vscode.EventEmitter();
public readonly onDidChangeCurrentIssueState: vscode.Event<void> = this._onDidChangeCurrentIssueState.event;

constructor(
private issueModel: IssueModel,
public readonly manager: FolderRepositoryManager,
Expand All @@ -47,7 +47,7 @@ export class CurrentIssue {
remote.name === repoRemote?.remoteName &&
remote.fetchUrl
?.toLowerCase()
.search(`${repoRemote.owner.toLowerCase()}/${repoRemote.repositoryName.toLowerCase()}`) !== -1
.includes(`${repoRemote.owner.toLowerCase()}/${repoRemote.repositoryName.toLowerCase()}`)
) {
this.repo = repo;
return;
Expand Down Expand Up @@ -79,19 +79,23 @@ export class CurrentIssue {
vscode.workspace.getConfiguration(ISSUES_SETTINGS_NAMESPACE).get(ASSIGN_WHEN_WORKING) &&
!this.issueModel.assignees?.find(value => value.login === login)
) {
// Check that we have a repo open for this issue and only try to assign in that case.
if (this.manager.gitHubRepositories.find(
r => r.remote.owner === this.issueModel.remote.owner && r.remote.repositoryName === this.issueModel.remote.repositoryName,
)) {
if (
this.manager.gitHubRepositories.find(
(r) =>
r.remote.owner === this.issueModel.remote.owner &&
r.remote.repositoryName === this.issueModel.remote.repositoryName,
)
) {
await this.manager.assignIssue(this.issueModel, login);
}
await this.stateManager.refresh(this.manager);
}
return true;
}
} catch (e) {
// leave repoDefaults undefined
vscode.window.showErrorMessage(vscode.l10n.t('There is no remote. Can\'t start working on an issue.'));
vscode.window.showErrorMessage(
vscode.l10n.t("There is no remote. Can't start working on an issue."),
);
}
return false;
}
Expand All @@ -107,10 +111,12 @@ export class CurrentIssue {
if (this._repoDefaults && checkoutDefaultBranch) {
try {
await this.manager.repository.checkout(this._repoDefaults.base);
} catch (e) {
} catch (e: any) {
if (e.gitErrorCode === GitErrorCodes.DirtyWorkTree) {
vscode.window.showErrorMessage(
vscode.l10n.t('Your local changes would be overwritten by checkout, please commit your changes or stash them before you switch branches'),
vscode.l10n.t(
'Your local changes would be overwritten by checkout, please commit your changes or stash them before you switch branches',
),
);
}
throw e;
Expand All @@ -120,63 +126,55 @@ export class CurrentIssue {
this.dispose();
}

private getBasicBranchName(user: string): string {
return `${user}/issue${this.issueModel.number}`;
}

private async getBranch(branch: string): Promise<Branch | undefined> {
try {
return await this.manager.repository.getBranch(branch);
} catch (e) {
// branch doesn't exist
} catch {
return undefined;
}
return undefined;
}

Copy link
Member

Choose a reason for hiding this comment

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

Checking that the branch name starts with origin/ isn't enough, as not all remotes will be named origin.

Suggested change
private getRemoteBranchPrefix(branch: string): string | undefined {
const remote = this.manager.repository.state.remotes.find((remote) => {
return branch.startsWith(`${remote.name}/`);
});
return remote ? `${remote.name}/` : undefined;
}

private async createOrCheckoutBranch(branch: string): Promise<boolean> {
let localBranchName = branch;
try {
if (await this.getBranch(branch)) {
await this.manager.repository.checkout(branch);
const isRemoteBranch = branch.startsWith('origin/');
if (isRemoteBranch) {
localBranchName = branch.substring('origin/'.length);
Comment on lines +135 to +137
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isRemoteBranch = branch.startsWith('origin/');
if (isRemoteBranch) {
localBranchName = branch.substring('origin/'.length);
const remoteBranchPrefix = this.getRemoteBranchPrefix(branch);
if (remoteBranchPrefix) {
localBranchName = branch.substring(remoteBranchPrefix.length);

}
const localBranch = await this.getBranch(localBranchName);
if (localBranch) {
await this.manager.repository.checkout(localBranchName);
} else if (isRemoteBranch) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} else if (isRemoteBranch) {
} else if (remoteBranchPrefix) {

await this.manager.repository.createBranch(localBranchName, true, branch);
await this.manager.repository.setBranchUpstream(localBranchName, branch);
} else {
await this.manager.repository.createBranch(branch, true);
await this.manager.repository.createBranch(localBranchName, true);
}
return true;
} catch (e) {
} catch (e: any) {
if (e.message !== 'User aborted') {
vscode.window.showErrorMessage(
`Unable to checkout branch ${branch}. There may be file conflicts that prevent this branch change. Git error: ${e.error}`,
`Unable to checkout branch ${localBranchName}. There may be file conflicts that prevent this branch change. Git error: ${e.message}`,
);
}
return false;
}
}

private async getUser(): Promise<string> {
if (!this.user) {
this.user = await this.issueModel.githubRepository.getAuthenticatedUser();
}
return this.user;
}

private async getBranchTitle(): Promise<string> {
return (
vscode.workspace.getConfiguration(ISSUES_SETTINGS_NAMESPACE).get<string>(ISSUE_BRANCH_TITLE) ??
amrshadid marked this conversation as resolved.
Show resolved Hide resolved
this.getBasicBranchName(await this.getUser())
);
}

private validateBranchName(branch: string): string | undefined {
const VALID_BRANCH_CHARACTERS = /[^ \\@\~\^\?\*\[]+/;
const match = branch.match(VALID_BRANCH_CHARACTERS);
if (match && match.length > 0 && match[0] !== branch) {
return vscode.l10n.t('Branch name cannot contain a space or the following characters: \\@~^?*[');
return vscode.l10n.t(
'Branch name cannot contain a space or the following characters: \\@~^?*[',
);
}
return undefined;
}

private showBranchNameError(error: string) {
const editSetting = `Edit Setting`;
vscode.window.showErrorMessage(error, editSetting).then(result => {
const editSetting = 'Edit Setting';
vscode.window.showErrorMessage(error, editSetting).then((result) => {
if (result === editSetting) {
vscode.commands.executeCommand(
'workbench.action.openSettings',
Expand All @@ -186,63 +184,124 @@ export class CurrentIssue {
});
}

private async offerNewBranch(branch: Branch, branchNameConfig: string, branchNameMatch: RegExpMatchArray | null | undefined): Promise<string> {
// Check if this branch has a merged PR associated with it.
// If so, offer to create a new branch.
const pr = await this.manager.getMatchingPullRequestMetadataFromGitHub(branch, branch.upstream?.remote, branch.upstream?.name);
if (pr && (pr.model.state !== GithubItemStateEnum.Open)) {
const mergedMessage = vscode.l10n.t('The pull request for {0} has been merged. Do you want to create a new branch?', branch.name ?? 'unknown branch');
const closedMessage = vscode.l10n.t('The pull request for {0} has been closed. Do you want to create a new branch?', branch.name ?? 'unknown branch');
const createBranch = vscode.l10n.t('Create New Branch');
const createNew = await vscode.window.showInformationMessage(pr.model.state === GithubItemStateEnum.Merged ? mergedMessage : closedMessage,
{
modal: true
}, createBranch);
if (createNew === createBranch) {
const number = (branchNameMatch?.length === 4 ? (Number(branchNameMatch[3]) + 1) : 1);
return `${branchNameConfig}_${number}`;
}
}
return branchNameConfig;
}

private async createIssueBranch(silent: boolean): Promise<boolean> {
alexr00 marked this conversation as resolved.
Show resolved Hide resolved
const createBranchConfig = this.shouldPromptForBranch
? 'prompt'
: vscode.workspace.getConfiguration(ISSUES_SETTINGS_NAMESPACE).get<string>(USE_BRANCH_FOR_ISSUES);
: vscode.workspace
.getConfiguration(ISSUES_SETTINGS_NAMESPACE)
.get<string>(USE_BRANCH_FOR_ISSUES);
if (createBranchConfig === 'off') {
return true;
}

const state: IssueState = this.stateManager.getSavedIssueState(this.issueModel.number);
this._branchName = this.shouldPromptForBranch ? undefined : state.branch;
const branchNameConfig = await variableSubstitution(
await this.getBranchTitle(),
this.issue,
undefined,
await this.getUser(),
const issueNumberStr = this.issueModel.number.toString();
const suggestedBranchName = `issue${issueNumberStr}`;
this._branchName = undefined;

const branches = await this.manager.repository.getBranches({ remote: true });
const branchesWithIssueNumber = branches.filter((branch) =>
branch.name?.includes(issueNumberStr),
);
const branchNameMatch = this._branchName?.match(new RegExp('^(' + branchNameConfig + ')(_)?(\\d*)'));
if ((createBranchConfig === 'on')) {
const branch = await this.getBranch(this._branchName!);
if (!branch) {
if (!branchNameMatch) {
this._branchName = branchNameConfig;
}
} else if (!silent) {
this._branchName = await this.offerNewBranch(branch, branchNameConfig, branchNameMatch);
const otherBranches = branches.filter(
(branch) => !branch.name?.includes(issueNumberStr),
);

const branchItems: vscode.QuickPickItem[] = [];

if (branchesWithIssueNumber.length > 0) {
branchItems.push({
label: 'Branches containing issue number:',
kind: vscode.QuickPickItemKind.Separator,
});
for (const branch of branchesWithIssueNumber) {
const isRemote = branch.name?.startsWith('origin/');
branchItems.push({
label: `${isRemote ? '$(cloud)' : '$(git-branch)'} ${branch.name ?? ''}`,
description: `${isRemote ? 'Remote' : 'Local'} branch`,
});
}
}
if (!this._branchName) {
this._branchName = await vscode.window.showInputBox({
value: branchNameConfig,
prompt: vscode.l10n.t('Enter the label for the new branch.'),

if (!branches.find((branch) => branch.name === suggestedBranchName)) {
branchItems.push({
label: `$(lightbulb) ${suggestedBranchName}`,
description: 'Suggested branch name for this issue',
detail: 'Recommended branch name based on the issue number',
picked: true,
});
}

if (otherBranches.length > 0) {
branchItems.push({
label: 'Other branches:',
kind: vscode.QuickPickItemKind.Separator,
});
for (const branch of otherBranches) {
const isRemote = branch.name?.startsWith('origin/');
branchItems.push({
label: `${isRemote ? '$(cloud)' : '$(git-branch)'} ${branch.name ?? ''}`,
description: `${isRemote ? 'Remote' : 'Local'} branch`,
});
}
}
if (!this._branchName) {
// user has cancelled

branchItems.push({
label: '$(pencil) Enter a custom branch name...',
description: 'Choose this to type your own branch name',
});

const quickPick = vscode.window.createQuickPick<vscode.QuickPickItem>();
Copy link
Member

Choose a reason for hiding this comment

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

Can you just use showQuickPick here instead of the more complicated createQuickPick?

quickPick.items = branchItems;
quickPick.placeholder = 'Select a branch or create a new one for this issue';
quickPick.ignoreFocusOut = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
quickPick.ignoreFocusOut = true;

quickPick.canSelectMany = false;

const suggestedItem = branchItems.find((item) =>
item.label.includes(suggestedBranchName),
);
if (suggestedItem) {
quickPick.activeItems = [suggestedItem];
}

quickPick.show();

const selectedBranch = await new Promise<vscode.QuickPickItem | undefined>((resolve) => {
quickPick.onDidAccept(() => {
const selection = quickPick.selectedItems[0];
resolve(selection);
quickPick.hide();
});
quickPick.onDidHide(() => {
resolve(undefined);
});
});

quickPick.dispose();

if (!selectedBranch) {
vscode.window.showInformationMessage('Branch selection cancelled.');
return false;
}

if (selectedBranch.label === '$(pencil) Enter a custom branch name...') {
const customBranchName = await vscode.window.showInputBox({
prompt: 'Enter your custom branch name',
placeHolder: 'e.g., feature/my-custom-branch',
validateInput: (input) =>
input.trim() === '' ? 'Branch name cannot be empty.' : undefined,
});

if (!customBranchName) {
vscode.window.showInformationMessage('Branch creation cancelled.');
return false;
}

this._branchName = customBranchName.trim();
} else {
this._branchName = selectedBranch.label.replace(/^\$\([^\)]+\)\s*/, '').trim();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of futzing with the label after the branch has been picked, you can extend QuickPickItem and add a new property to it to track the actual branch name associated with the quick pick item.

}

const validateBranchName = this.validateBranchName(this._branchName);
if (validateBranchName) {
this.showBranchNameError(validateBranchName);
Expand All @@ -251,15 +310,19 @@ export class CurrentIssue {

state.branch = this._branchName;
await this.stateManager.setSavedIssueState(this.issueModel, state);

if (!(await this.createOrCheckoutBranch(this._branchName))) {
this._branchName = undefined;
return false;
}

return true;
}

public async getCommitMessage(): Promise<string | undefined> {
const configuration = vscode.workspace.getConfiguration(ISSUES_SETTINGS_NAMESPACE).get(WORKING_ISSUE_FORMAT_SCM);
const configuration = vscode.workspace
.getConfiguration(ISSUES_SETTINGS_NAMESPACE)
.get(WORKING_ISSUE_FORMAT_SCM);
if (typeof configuration === 'string') {
return variableSubstitution(configuration, this.issueModel, this._repoDefaults);
}
Expand All @@ -271,6 +334,5 @@ export class CurrentIssue {
if (this.repo && message) {
this.repo.inputBox.value = message;
}
return;
}
}
}