Skip to content

Commit f5d0f99

Browse files
Copilotalexr00
andcommitted
Address code review feedback: extract helper and improve error handling
Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>
1 parent d7859a3 commit f5d0f99

File tree

2 files changed

+22
-24
lines changed

2 files changed

+22
-24
lines changed

src/@types/vscode.proposed.chatParticipantAdditions.d.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,6 @@ declare module 'vscode' {
105105
isComplete?: boolean;
106106
toolSpecificData?: ChatTerminalToolInvocationData;
107107
fromSubAgent?: boolean;
108-
presentation?: 'hidden' | 'hiddenAfterComplete' | undefined;
109108

110109
constructor(toolName: string, toolCallId: string, isError?: boolean);
111110
}

src/github/pullRequestReviewCommon.ts

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ export namespace PullRequestReviewCommon {
345345
const deletedBranchTypes: string[] = [];
346346

347347
if (selectedActions) {
348-
const isBranchActive = item.equals(folderRepositoryManager.activePullRequest) || (folderRepositoryManager.repository.state.HEAD?.name && folderRepositoryManager.repository.state.HEAD.name === branchInfo?.branch);
348+
const isBranchActiveForDeletion = branchInfo && isBranchActive(folderRepositoryManager, item, branchInfo.branch);
349349

350350
const promises = selectedActions.map(async action => {
351351
switch (action.type) {
@@ -359,7 +359,7 @@ export namespace PullRequestReviewCommon {
359359
}
360360
return;
361361
case 'local':
362-
if (isBranchActive) {
362+
if (isBranchActiveForDeletion) {
363363
if (folderRepositoryManager.repository.state.workingTreeChanges.length) {
364364
const yes = vscode.l10n.t('Yes');
365365
const response = await vscode.window.showWarningMessage(
@@ -405,6 +405,14 @@ export namespace PullRequestReviewCommon {
405405
}
406406
}
407407

408+
/**
409+
* Check if a pull request's branch is currently active (checked out).
410+
*/
411+
function isBranchActive(folderRepositoryManager: FolderRepositoryManager, item: PullRequestModel, branchName: string): boolean {
412+
return item.equals(folderRepositoryManager.activePullRequest) ||
413+
(folderRepositoryManager.repository.state.HEAD?.name === branchName);
414+
}
415+
408416
/**
409417
* Automatically delete branches after merge based on user preferences.
410418
* This function does not show any prompts - it uses the default deletion method preferences.
@@ -422,50 +430,41 @@ export namespace PullRequestReviewCommon {
422430
.getConfiguration(PR_SETTINGS_NAMESPACE)
423431
.get<boolean>(`${DEFAULT_DELETION_METHOD}.${SELECT_REMOTE}`, true);
424432

425-
const promises: Promise<void>[] = [];
433+
const deletionTasks: Promise<void>[] = [];
426434

427435
// Delete remote head branch if it's not the default branch
428436
if (item.isResolved()) {
429437
const isDefaultBranch = defaultBranch === item.head.ref;
430438
if (!isDefaultBranch && !item.isRemoteHeadDeleted) {
431-
promises.push(
432-
folderRepositoryManager.deleteBranch(item).then(() => {
433-
return folderRepositoryManager.repository.fetch({ prune: true });
434-
}).catch(e => {
435-
Logger.warn(`Failed to delete remote branch for PR #${item.number}: ${e}`, 'PullRequestReviewCommon');
436-
})
439+
deletionTasks.push(
440+
folderRepositoryManager.deleteBranch(item)
441+
.then(() => folderRepositoryManager.repository.fetch({ prune: true }))
442+
.catch(e => Logger.warn(`Failed to delete remote branch for PR #${item.number}: ${e}`, 'PullRequestReviewCommon'))
437443
);
438444
}
439445
}
440446

441447
// Delete local branch if preference is set
442448
if (branchInfo && deleteLocalBranch) {
443-
const isBranchActive = item.equals(folderRepositoryManager.activePullRequest) ||
444-
(folderRepositoryManager.repository.state.HEAD?.name && folderRepositoryManager.repository.state.HEAD.name === branchInfo.branch);
445-
446-
promises.push(
449+
deletionTasks.push(
447450
(async () => {
448-
if (isBranchActive) {
449-
// Checkout default branch before deleting the active branch
451+
if (isBranchActive(folderRepositoryManager, item, branchInfo.branch)) {
450452
await folderRepositoryManager.checkoutDefaultBranch(defaultBranch);
451453
}
452454
await folderRepositoryManager.repository.deleteBranch(branchInfo.branch, true);
453-
})().catch(e => {
454-
Logger.warn(`Failed to delete local branch ${branchInfo.branch} for PR #${item.number}: ${e}`, 'PullRequestReviewCommon');
455-
})
455+
})().catch(e => Logger.warn(`Failed to delete local branch ${branchInfo.branch} for PR #${item.number}: ${e}`, 'PullRequestReviewCommon'))
456456
);
457457
}
458458

459459
// Delete remote if it's no longer used and preference is set
460460
if (branchInfo && branchInfo.remote && branchInfo.createdForPullRequest && !branchInfo.remoteInUse && deleteRemote) {
461-
promises.push(
462-
folderRepositoryManager.repository.removeRemote(branchInfo.remote).catch(e => {
463-
Logger.warn(`Failed to delete remote ${branchInfo.remote} for PR #${item.number}: ${e}`, 'PullRequestReviewCommon');
464-
})
461+
deletionTasks.push(
462+
folderRepositoryManager.repository.removeRemote(branchInfo.remote)
463+
.catch(e => Logger.warn(`Failed to delete remote ${branchInfo.remote} for PR #${item.number}: ${e}`, 'PullRequestReviewCommon'))
465464
);
466465
}
467466

468467
// Execute all deletions in parallel
469-
await Promise.all(promises);
468+
await Promise.all(deletionTasks);
470469
}
471470
}

0 commit comments

Comments
 (0)