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 all 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
194 changes: 129 additions & 65 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 @@ -16,7 +16,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 Down Expand Up @@ -130,19 +129,30 @@ export class CurrentIssue extends Disposable {
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> {
private async createOrCheckoutBranch(branch: string, silent: boolean): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

Does this really need a silent? It seems like something that would always be useful to know if it failed.

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);
console.log('Setting upstream');
console.log(localBranchName, '->', 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}`,
);
if (!silent) {
vscode.window.showErrorMessage(`Unable to checkout branch ${localBranchName}. There may be file conflicts that prevent this branch change. Git error: ${e.message}`);
}
}
return false;
}
Expand All @@ -156,10 +166,12 @@ export class CurrentIssue extends Disposable {
}

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())
);
const branchTitleSetting = vscode.workspace.getConfiguration(ISSUES_SETTINGS_NAMESPACE).get<string>(ISSUE_BRANCH_TITLE);
const branchTitle = branchTitleSetting
? await variableSubstitution(branchTitleSetting, this.issue, undefined, await this.getUser())
: this.getBasicBranchName(await this.getUser());

return branchTitle;
}

private validateBranchName(branch: string): string | undefined {
Expand All @@ -171,9 +183,12 @@ export class CurrentIssue extends Disposable {
return undefined;
}

private showBranchNameError(error: string) {
const editSetting = `Edit Setting`;
vscode.window.showErrorMessage(error, editSetting).then(result => {
private showBranchNameError(error: string, silent: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Same for this. I think if the branch name is bad then we should always show an error.

if (silent) {
return;
}
const editSetting = 'Edit Setting';
vscode.window.showErrorMessage(error, editSetting).then((result) => {
if (result === editSetting) {
vscode.commands.executeCommand(
'workbench.action.openSettings',
Expand All @@ -183,75 +198,124 @@ export class CurrentIssue extends Disposable {
});
}

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);
if (createBranchConfig === 'off') {
return true;
}

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 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 issueNumberStr = this.issueModel.number.toString();
const issueTitle = this.issueModel.title;
const suggestedBranchName = (await this.getBranchTitle()).toLocaleLowerCase();
Comment on lines +210 to +211
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 issueTitle = this.issueModel.title;
const suggestedBranchName = (await this.getBranchTitle()).toLocaleLowerCase();
const suggestedBranchName = await this.getBranchTitle();
// eslint-disable-next-line no-template-curly-in-string
const testBranchFromIssueTitle = await variableSubstitution('${issueTitle}', this.issueModel, undefined, await this.getUser()));


this._branchName = undefined;

const branches = await this.manager.repository.getBranches({ remote: true });
const branchesWithIssueMatch: vscode.QuickPickItem[] = [];
const otherBranches: vscode.QuickPickItem[] = [];

branches.forEach((branch) => {
const isRemote = branch.name?.startsWith('origin/');
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 isRemote = branch.name?.startsWith('origin/');
const isRemote = !!branch.remote;

const displayName = branch.name ?? '';
const branchItem = {
label: `${isRemote ? '$(cloud)' : '$(git-branch)'} ${displayName}`,
description: `${isRemote ? 'Remote' : 'Local'} branch`,
};

if (
branch.name?.toLowerCase().includes(issueNumberStr.toLowerCase()) ||
branch.name?.toLowerCase().includes(issueTitle.replace(/[^a-zA-Z0-9-]/g, '-').toLowerCase()) ||
branch.name?.toLowerCase().includes(issueTitle.replace(/[^a-zA-Z0-9-]/g, '_').toLowerCase())
Comment on lines +227 to +230
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
if (
branch.name?.toLowerCase().includes(issueNumberStr.toLowerCase()) ||
branch.name?.toLowerCase().includes(issueTitle.replace(/[^a-zA-Z0-9-]/g, '-').toLowerCase()) ||
branch.name?.toLowerCase().includes(issueTitle.replace(/[^a-zA-Z0-9-]/g, '_').toLowerCase())
if ((branch.name !== suggestedBranchName) &&
(branch.name?.toLowerCase().includes(issueNumberStr.toLowerCase()) ||
branch.name?.toLowerCase().includes(testBranchFromIssueTitle.toLowerCase()))

) {
branchesWithIssueMatch.push(branchItem);
} else {
otherBranches.push(branchItem);
}
});

// Create QuickPick items
const branchItems: vscode.QuickPickItem[] = [
{ label: 'Suggested branch', kind: vscode.QuickPickItemKind.Separator },
Copy link
Member

Choose a reason for hiding this comment

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

All the strings should be localized with vscode.l10n.t

{
label: `$(lightbulb) ${suggestedBranchName}`,
description: '',
detail: 'Recommended branch name based on settings',
picked: true,
},
];

if (branchesWithIssueMatch.length > 0) {
branchItems.push({ label: 'Branches matching', kind: vscode.QuickPickItemKind.Separator });
branchItems.push(...branchesWithIssueMatch);
}
if (!this._branchName) {
this._branchName = await vscode.window.showInputBox({
value: branchNameConfig,
prompt: vscode.l10n.t('Enter the label for the new branch.'),
});

if (otherBranches.length > 0) {
branchItems.push({ label: 'Other branches', kind: vscode.QuickPickItemKind.Separator });
branchItems.push(...otherBranches);
}
if (!this._branchName) {
// user has cancelled

branchItems.push(
{ label: 'Custom branch name:', kind: vscode.QuickPickItemKind.Separator },
{ label: '$(pencil) Enter a custom branch name...', description: 'Choose this to type your own branch name' }
Copy link
Member

Choose a reason for hiding this comment

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

Since this string is used again further down, it would be better to move it into a const, especially after it's been localized.

);

// Show QuickPick for branch selection
Copy link
Member

Choose a reason for hiding this comment

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

We should only show the quick pick if createBranchConfig === 'prompt.

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.activeItems = [branchItems[1]];
quickPick.show();

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

quickPick.dispose();

if (!selectedBranch) {
if (!silent) 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',
value: suggestedBranchName,
validateInput: (input) => (input.trim() === '' ? 'Branch name cannot be empty.' : undefined),
});

if (!customBranchName) {
if (!silent) 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);
this.showBranchNameError(validateBranchName, silent);
return false;
}

state.branch = this._branchName;
await this.stateManager.setSavedIssueState(this.issueModel, state);
if (!(await this.createOrCheckoutBranch(this._branchName))) {

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

return true;
}

Expand All @@ -270,4 +334,4 @@ export class CurrentIssue extends Disposable {
}
return;
}
}
}