Merge pull request #11150 from gitbutlerapp/branches-stale-selection

Clean-up stale branch selection

Author: estib-vega

apps/desktop/cypress/e2e/support/mock/upstreamIntegration.ts

@@ -5,6 +5,5 @@ export const MOCK_BRANCH_STATUSES_RESPONSE: BranchStatusesResponse = {
 };
 
 export const MOCK_INTEGRATION_OUTCOME: IntegrationOutcome = {
-	archivedBranches: [],
-	reviewIdsToClose: []
+	deletedBranches: []
 };

apps/desktop/src/components/BranchesView.svelte

@@ -192,7 +192,7 @@
 			current.stackId || (current.branchName && !isTargetBranch)}
 		{@const isNonLocalPr = !isStackOrNormalBranchPreview && current.prNumber !== undefined}
 
-		<div class="branches-view">
+		<div class="branches-view" data-testid={TestId.BranchesView}>
 			<div class="relative overflow-hidden radius-ml">
 				<div
 					bind:this={branchViewLeftEl}

apps/desktop/src/lib/bootstrap/deps.ts

@@ -262,7 +262,7 @@ export function initDependencies(args: {
 	const upstreamIntegrationService = new UpstreamIntegrationService(
 		clientState,
 		stackService,
-		projectsService
+		uiState
 	);
 
 	// ============================================================================

apps/desktop/src/lib/state/uiState.svelte.ts

@@ -527,3 +527,15 @@ export function updateStalePrSelection(uiState: UiState, projectId: string, prs:
 		projectState.branchesSelection.set({});
 	}
 }
+
+export function updateStaleStackSelectionInBranchesView(
+	uiState: UiState,
+	projectId: string,
+	deletedBranches: string[]
+) {
+	const projectState = uiState.project(projectId);
+	const selection = projectState.branchesSelection.current;
+	if (selection.branchName && deletedBranches.includes(selection.branchName)) {
+		projectState.branchesSelection.set({});
+	}
+}

apps/desktop/src/lib/upstream/types.ts

@@ -63,8 +63,7 @@ export type BaseBranchResolution = {
 };
 
 export type IntegrationOutcome = {
-	archivedBranches: string[];
-	reviewIdsToClose: string[];
+	deletedBranches: string[];
 };
 
 export function getBaseBranchResolution(

apps/desktop/src/lib/upstream/upstreamIntegrationService.svelte.ts

@@ -1,5 +1,5 @@
-import { ProjectsService } from '$lib/project/projectsService';
 import { invalidatesList, providesList, ReduxTag } from '$lib/state/tags';
+import { updateStaleStackSelectionInBranchesView, type UiState } from '$lib/state/uiState.svelte';
 import { InjectionToken } from '@gitbutler/core/context';
 import { isDefined } from '@gitbutler/ui/utils/typeguards';
 import type { StackService } from '$lib/stacks/stackService.svelte';
@@ -23,9 +23,9 @@ export class UpstreamIntegrationService {
 	constructor(
 		state: ClientState,
 		private stackService: StackService,
-		private projectsService: ProjectsService
+		uiState: UiState
 	) {
-		this.api = injectEndpoints(state.backendApi);
+		this.api = injectEndpoints(state.backendApi, uiState);
 	}
 
 	async upstreamStatuses(
@@ -72,7 +72,7 @@ export class UpstreamIntegrationService {
 	}
 }
 
-function injectEndpoints(api: ClientState['backendApi']) {
+function injectEndpoints(api: ClientState['backendApi'], uiState: UiState) {
 	return api.injectEndpoints({
 		endpoints: (build) => ({
 			upstreamIntegrationStatuses: build.query<
@@ -99,8 +99,13 @@ function injectEndpoints(api: ClientState['backendApi']) {
 				invalidatesTags: [
 					invalidatesList(ReduxTag.UpstreamIntegrationStatus),
 					invalidatesList(ReduxTag.Stacks),
-					invalidatesList(ReduxTag.StackDetails)
-				]
+					invalidatesList(ReduxTag.StackDetails),
+					invalidatesList(ReduxTag.BranchListing)
+				],
+				transformResponse: (response: IntegrationOutcome, _, { projectId }) => {
+					updateStaleStackSelectionInBranchesView(uiState, projectId, response.deletedBranches);
+					return response;
+				}
 			}),
 			resolveUpstreamIntegration: build.mutation<
 				string,

crates/gitbutler-branch-actions/src/upstream_integration.rs

@@ -98,10 +98,8 @@ pub struct BaseBranchResolution {
 #[derive(Serialize, Deserialize, PartialEq, Debug)]
 #[serde(rename_all = "camelCase")]
 pub struct IntegrationOutcome {
-    /// This is the list of branch names that have become archived as a result of the upstream integration
-    archived_branches: Vec<String>,
-    /// This is the list of review ids that have been closed as a result of the upstream integration
-    review_ids_to_close: Vec<String>,
+    /// The list of branches that have been deleted as a result of the upstream integration
+    deleted_branches: Vec<String>,
 }
 
 impl StackStatus {
@@ -468,8 +466,7 @@ pub(crate) fn integrate_upstream(
     let virtual_branches_state = VirtualBranchesHandle::new(ctx.project().gb_dir());
     let default_target = virtual_branches_state.get_default_target()?;
 
-    let mut newly_archived_branches = vec![];
-    let mut to_be_closed_review_ids = vec![];
+    let mut deleted_branches = vec![];
 
     // Ensure resolutions match current statuses
     {
@@ -544,7 +541,15 @@ pub(crate) fn integrate_upstream(
 
             if delete_local_refs {
                 for head in &stack.heads {
-                    head.delete_reference(&gix_repo).ok();
+                    let branch_name = head.name.to_str().context("Invalid branch name")?;
+                    match head.delete_reference(&gix_repo) {
+                        Ok(_) => {
+                            deleted_branches.push(branch_name.to_string());
+                        }
+                        _ => {
+                            // Fail silently because interrupting this is worse
+                        }
+                    }
                 }
             }
         }
@@ -611,10 +616,9 @@ pub(crate) fn integrate_upstream(
                 .map(|r| r.delete_integrated_branches)
                 .unwrap_or(false);
 
-            let (mut archived_branches, mut review_ids_to_close) =
+            let stack_branches_deleted =
                 stack.archive_integrated_heads(ctx, &gix_repo, for_archival, delete_local_refs)?;
-            newly_archived_branches.append(&mut archived_branches);
-            to_be_closed_review_ids.append(&mut review_ids_to_close);
+            deleted_branches.extend(stack_branches_deleted);
         }
 
         {
@@ -625,10 +629,10 @@ pub(crate) fn integrate_upstream(
         crate::integration::update_workspace_commit(&virtual_branches_state, ctx, false)?;
     }
 
-    Ok(IntegrationOutcome {
-        archived_branches: newly_archived_branches,
-        review_ids_to_close: to_be_closed_review_ids,
-    })
+    deleted_branches.sort();
+    deleted_branches.dedup();
+
+    Ok(IntegrationOutcome { deleted_branches })
 }
 
 pub(crate) fn resolve_upstream_integration(

crates/gitbutler-stack/src/stack.rs

@@ -663,11 +663,10 @@ impl Stack {
         repo: &gix::Repository,
         for_archival: &[Reference],
         delete_local_refs: bool,
-    ) -> Result<(Vec<String>, Vec<String>)> {
+    ) -> Result<Vec<String>> {
         self.ensure_initialized()?;
 
-        let mut newly_archived_branches = vec![];
-        let mut review_ids_to_close = vec![];
+        let mut deleted_branches = vec![];
 
         self.updated_timestamp_ms = gitbutler_time::time::now_ms();
         let state = branch_state(ctx);
@@ -678,13 +677,16 @@ impl Stack {
                 Reference::Virtual(r) => r == head.name(),
             }) {
                 head.archived = true;
-                newly_archived_branches.push(head.name().clone());
-                if let Some(review_id) = head.review_id.clone() {
-                    review_ids_to_close.push(review_id);
-                }
 
                 if delete_local_refs {
-                    head.delete_reference(repo).ok(); // Fail silently because interrupting this is worse
+                    match head.delete_reference(repo) {
+                        Ok(_) => {
+                            deleted_branches.push(head.name().clone());
+                        }
+                        Err(_) => {
+                            // Ignore errors when deleting references
+                        }
+                    }
                 }
             }
         }
@@ -701,7 +703,7 @@ impl Stack {
 
         state.set_stack(self.clone())?;
 
-        Ok((newly_archived_branches, review_ids_to_close))
+        Ok(deleted_branches)
     }
 
     /// Prepares push details according to the series to be pushed (picking out the correct sha and remote refname)

e2e/playwright/scripts/merge-upstream-branch-to-base.sh

@@ -3,9 +3,11 @@
 echo "GIT CONFIG $GIT_CONFIG_GLOBAL"
 echo "DATA DIR $GITBUTLER_CLI_DATA_DIR"
 echo "BUT_TESTING $BUT_TESTING"
-echo "BRANCH TO APPLY: $1"
-# Apply remote branch to the workspace.
+echo "BRANCH TO MERGE: $1"
+
+# Merge the upstream branch into master and delete the upstream branch
 pushd remote-project
   git checkout master
   git merge --no-ff -m "Merging upstream branch $1 into base" "$1"
+  git branch -d "$1"
 popd

e2e/playwright/tests/branches.spec.ts

@@ -574,3 +574,70 @@ test('should handle gracefully applying two conflicting branches', async ({
 	// The modal explaining this should be visible
 	await waitForTestId(page, 'stacks-unapplied-toast');
 });
+
+test('should update the stale selection of an unexisting branch', async ({
+	page,
+	context
+}, testInfo) => {
+	const workdir = testInfo.outputPath('workdir');
+	const configdir = testInfo.outputPath('config');
+	gitbutler = await startGitButler(workdir, configdir, context);
+
+	await gitbutler.runScript('project-with-remote-branches.sh');
+	// Apply branch1
+	await gitbutler.runScript('apply-upstream-branch.sh', ['branch1', 'local-clone']);
+
+	await page.goto('/');
+
+	// Should load the workspace
+	await waitForTestId(page, 'workspace-view');
+
+	// Navigate to branches page
+	await clickByTestId(page, 'navigation-branches-button');
+	let header = await waitForTestId(page, 'target-commit-list-header');
+
+	await expect(header).toContainText('origin/master');
+
+	let branchListCards = getByTestId(page, 'branch-list-card');
+	await expect(branchListCards).toHaveCount(3);
+
+	// Select the branch1
+	let firstBranchCard = branchListCards.filter({ hasText: 'branch1' });
+	await expect(firstBranchCard).toBeVisible();
+	await firstBranchCard.click();
+
+	// Go back to the workspace
+	await clickByTestId(page, 'navigation-workspace-button');
+	await waitForTestId(page, 'workspace-view');
+
+	// There should be one stack applied
+	const stacks = getByTestId(page, 'stack');
+	await expect(stacks).toHaveCount(1);
+
+	// Branch one was merged in the forge
+	await gitbutler.runScript('merge-upstream-branch-to-base.sh', ['branch1']);
+
+	// Click the sync button
+	await clickByTestId(page, 'sync-button');
+
+	// Update the workspace
+	await clickByTestId(page, 'integrate-upstream-commits-button');
+	await clickByTestId(page, 'integrate-upstream-action-button');
+	await waitForTestIdToNotExist(page, 'integrate-upstream-action-button');
+
+	// There should be no stacks
+	await waitForTestIdToNotExist(page, 'stack');
+
+	// Navigate to branches page
+	await clickByTestId(page, 'navigation-branches-button');
+	await waitForTestId(page, 'branches-view');
+
+	header = await waitForTestId(page, 'target-commit-list-header');
+
+	await expect(header).toContainText('origin/master');
+	// The previously selected branch1 should not be selected anymore
+	branchListCards = getByTestId(page, 'branch-list-card');
+	await expect(branchListCards).toHaveCount(2);
+	firstBranchCard = branchListCards.filter({ hasText: 'branch1' });
+	await expect(firstBranchCard).not.toBeVisible();
+});