From bc50c35bc8613ddf01c6fa241623fa1410341c65 Mon Sep 17 00:00:00 2001 From: Yuruzuu Date: Sun, 1 Nov 2026 16:02:21 +0800 Subject: [PATCH] perf(server,web): branch switches stop flipping back and redoing PR lookups Co-Authored-By: Claude Opus 5.5 --- apps/server/src/git/GitManager.test.ts | 36 +++++ apps/server/src/git/GitManager.ts | 10 +- apps/server/src/git/GitWorkflowService.ts | 4 +- apps/server/src/vcs/GitVcsDriver.ts | 2 + apps/server/src/vcs/GitVcsDriverCore.test.ts | 25 +++ apps/server/src/vcs/GitVcsDriverCore.ts | 18 +++ .../src/vcs/VcsStatusBroadcaster.test.ts | 145 +++++++++++++++++- apps/server/src/vcs/VcsStatusBroadcaster.ts | 50 ++++++ apps/server/src/ws.ts | 14 +- .../components/BranchToolbar.logic.test.ts | 19 +++ .../web/src/components/BranchToolbar.logic.ts | 17 ++ .../BranchToolbarBranchSelector.tsx | 20 ++- 12 files changed, 351 insertions(+), 9 deletions(-) diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index a71f19e6a973b..d60d25cfa172c 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -2433,6 +2433,42 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { }), ); + it.effect("a checkout's invalidation keeps the branch's cached PR lookup", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-cached"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/pr-cached"]); + + const { manager } = yield* makeManager({ + providerScenario: { + prListSequence: [ + [ + { + number: 215, + title: "Cached PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/215", + baseRefName: "main", + headRefName: "feature/pr-cached", + }, + ], + [], + ], + }, + }); + + expect((yield* manager.status({ cwd: repoDir })).pr?.number).toBe(215); + + yield* manager.invalidateStatus(repoDir, { keepPullRequestLookups: true }); + expect((yield* manager.status({ cwd: repoDir })).pr?.number).toBe(215); + + yield* manager.invalidateStatus(repoDir); + expect((yield* manager.status({ cwd: repoDir })).pr).toBeNull(); + }), + ); + it.effect( "status does not reuse a stale PR after the branch is retargeted to a different upstream", () => diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts index 95f5187b1a303..86c94ddc21412 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -140,7 +140,11 @@ export class GitManager extends Context.Service< ) => Effect.Effect; readonly invalidateLocalStatus: (cwd: string) => Effect.Effect; readonly invalidateRemoteStatus: (cwd: string) => Effect.Effect; - readonly invalidateStatus: (cwd: string) => Effect.Effect; + /** keepPullRequestLookups is for changes that cannot affect a PR, such as a checkout. */ + readonly invalidateStatus: ( + cwd: string, + options?: { readonly keepPullRequestLookups?: boolean }, + ) => Effect.Effect; readonly resolvePullRequest: ( input: GitPullRequestRefInput, ) => Effect.Effect; @@ -2665,10 +2669,10 @@ export const make = Effect.gen(function* () { yield* invalidateRemoteStatusResultCache(cwd); }); const invalidateStatus: GitManager["Service"]["invalidateStatus"] = Effect.fn("invalidateStatus")( - function* (cwd) { + function* (cwd, options) { yield* invalidateLocalStatusResultCache(cwd); yield* invalidateRemoteStatusResultCache(cwd); - yield* bumpPrLookupEpoch(cwd); + if (!options?.keepPullRequestLookups) yield* bumpPrLookupEpoch(cwd); yield* forgetPrLookupGitState(cwd); }, ); diff --git a/apps/server/src/git/GitWorkflowService.ts b/apps/server/src/git/GitWorkflowService.ts index fdc00849af2d0..89ae147ce9f29 100644 --- a/apps/server/src/git/GitWorkflowService.ts +++ b/apps/server/src/git/GitWorkflowService.ts @@ -59,7 +59,8 @@ export class GitWorkflowService extends Context.Service< ) => Effect.Effect; readonly invalidateLocalStatus: (cwd: string) => Effect.Effect; readonly invalidateRemoteStatus: (cwd: string) => Effect.Effect; - readonly invalidateStatus: (cwd: string) => Effect.Effect; + readonly invalidateStatus: GitManager.GitManager["Service"]["invalidateStatus"]; + readonly statusUpstreamFetchCount: GitVcsDriver.GitVcsDriver["Service"]["statusUpstreamFetchCount"]; readonly pullCurrentBranch: (cwd: string) => Effect.Effect; readonly runStackedAction: ( input: GitRunStackedActionInput, @@ -369,6 +370,7 @@ export const make = Effect.gen(function* () { invalidateLocalStatus: gitManager.invalidateLocalStatus, invalidateRemoteStatus: gitManager.invalidateRemoteStatus, invalidateStatus: gitManager.invalidateStatus, + statusUpstreamFetchCount: git.statusUpstreamFetchCount, pullCurrentBranch: Effect.fn("GitWorkflowService.pullCurrentBranch")(function (cwd) { return ensureGitCommand("GitWorkflowService.pullCurrentBranch", cwd).pipe( Effect.andThen(git.pullCurrentBranch(cwd)), diff --git a/apps/server/src/vcs/GitVcsDriver.ts b/apps/server/src/vcs/GitVcsDriver.ts index fe49a6b1391af..65dc2d183a3ce 100644 --- a/apps/server/src/vcs/GitVcsDriver.ts +++ b/apps/server/src/vcs/GitVcsDriver.ts @@ -310,6 +310,8 @@ export class GitVcsDriver extends Context.Service< cwd: string, options?: GitRemoteStatusOptions, ) => Effect.Effect; + /** Changes whenever a status read fetches this repository, so callers can tell if refs may have moved. */ + readonly statusUpstreamFetchCount: (cwd: string) => Effect.Effect; readonly prepareCommitContext: ( cwd: string, filePaths?: readonly string[], diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index d9a48457f8a38..534f66321554f 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -3386,6 +3386,31 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => { }), ); + it.effect("counts a status read's upstream fetch only when one actually ran", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const remote = yield* makeTmpDir("git-vcs-driver-remote-"); + const { initialBranch } = yield* initRepoWithCommit(cwd); + yield* git(remote, ["init", "--bare"]); + yield* git(cwd, ["remote", "add", "origin", remote]); + yield* git(cwd, ["push", "-u", "origin", initialBranch]); + + const driver = yield* GitVcsDriver.GitVcsDriver; + const initial = yield* driver.statusUpstreamFetchCount(cwd); + + yield* driver.statusDetailsRemote(cwd, { refreshUpstream: false }); + assert.equal(yield* driver.statusUpstreamFetchCount(cwd), initial); + + yield* driver.statusDetailsRemote(cwd); + const afterFetch = yield* driver.statusUpstreamFetchCount(cwd); + assert.equal(afterFetch, initial + 1); + + // Still inside the refresh interval, so this read reuses the last fetch. + yield* driver.statusDetailsRemote(cwd); + assert.equal(yield* driver.statusUpstreamFetchCount(cwd), afterFetch); + }), + ); + it.effect("does not start Git auto-maintenance from background upstream fetches", () => Effect.gen(function* () { const cwd = yield* makeTmpDir(); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 11580f4c26e18..c415f207a9285 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -1621,10 +1621,20 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* } } }); + const statusUpstreamFetches = new Map(); const refreshStatusRemoteCacheEntry = Effect.fn("refreshStatusRemoteCacheEntry")(function* ( cacheKey: StatusRemoteRefreshCacheKey, ) { return yield* fetchRemoteForStatus(cacheKey.gitCommonDir, cacheKey.remoteName).pipe( + // Counted on failure too: a fetch can move some refs before it fails. + Effect.ensuring( + Effect.sync(() => { + statusUpstreamFetches.set( + cacheKey.gitCommonDir, + (statusUpstreamFetches.get(cacheKey.gitCommonDir) ?? 0) + 1, + ); + }), + ), Effect.as(true as const), ); }); @@ -1640,6 +1650,13 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ), }); + const statusUpstreamFetchCount: GitVcsDriver.GitVcsDriver["Service"]["statusUpstreamFetchCount"] = + (cwd) => + resolveGitCommonDir(cwd).pipe( + Effect.map((gitCommonDir) => statusUpstreamFetches.get(gitCommonDir) ?? 0), + Effect.orElseSucceed(() => 0), + ); + const refreshStatusUpstreamIfStale = Effect.fn("refreshStatusUpstreamIfStale")(function* ( cwd: string, ) { @@ -4936,6 +4953,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* statusDetails, statusDetailsLocal, statusDetailsRemote, + statusUpstreamFetchCount, prepareCommitContext, commit: Effect.fnUntraced(function* (cwd, subject, body, options) { return yield* withListRefsInvalidation(cwd, commit(cwd, subject, body, options)); diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts index 042f8274543e5..00518490f47b6 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts @@ -23,6 +23,7 @@ import type { VcsStatusStreamEvent, } from "@t3tools/contracts"; import { GitCommandError, GitManagerError } from "@t3tools/contracts"; +import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import * as VcsStatusBroadcaster from "./VcsStatusBroadcaster.ts"; import * as BackgroundPolicy from "../background/BackgroundPolicy.ts"; @@ -72,6 +73,9 @@ function layerTestFor(state: { localInvalidationCalls: number; remoteInvalidationCalls: number; remoteStatusRefreshUpstreamValues?: Array; + remoteStatusInteractiveValues?: Array; + invalidateStatusKeepLookupValues?: Array; + upstreamFetchCount?: number; remoteStatusError?: GitManagerError; backgroundWorkEnabled?: boolean; automaticFetchInterval?: Effect.Effect; @@ -113,6 +117,13 @@ function layerTestFor(state: { return state.currentRemoteStatus; }), ), + Effect.tap(() => + Effect.gen(function* () { + state.remoteStatusInteractiveValues?.push( + yield* SourceControlRateLimit.Interactive, + ); + }), + ), Effect.tap(() => state.remoteStatusError ? Effect.fail(state.remoteStatusError) : Effect.void, ), @@ -126,11 +137,13 @@ function layerTestFor(state: { Effect.sync(() => { state.remoteInvalidationCalls += 1; }), - invalidateStatus: () => + invalidateStatus: (_cwd, options) => Effect.sync(() => { state.localInvalidationCalls += 1; state.remoteInvalidationCalls += 1; + state.invalidateStatusKeepLookupValues?.push(options?.keepPullRequestLookups); }), + statusUpstreamFetchCount: () => Effect.sync(() => state.upstreamFetchCount ?? 0), ...(state.pullCurrentBranch ? { pullCurrentBranch: state.pullCurrentBranch } : {}), }), ), @@ -656,6 +669,136 @@ describe("VcsStatusBroadcaster", () => { }, ); + it.effect("publishes a checkout's branch before returning, ahead of the remote read", () => { + const state: Parameters[0] = { + currentLocalStatus: baseLocalStatus, + currentRemoteStatus: baseRemoteStatus, + localStatusCalls: 0, + remoteStatusCalls: 0, + localInvalidationCalls: 0, + remoteInvalidationCalls: 0, + }; + + return Effect.gen(function* () { + const broadcaster = yield* VcsStatusBroadcaster.VcsStatusBroadcaster; + yield* broadcaster.getStatus({ cwd: "/repo" }); + const subscribed = yield* Deferred.make(); + const localPublished = yield* Deferred.make(); + const remotePublished = yield* Deferred.make(); + yield* broadcaster.streamStatus({ cwd: "/repo", includeRemote: false }).pipe( + Stream.runForEach((event) => { + if (event._tag === "snapshot") return Deferred.succeed(subscribed, undefined); + if (event._tag === "localUpdated") return Deferred.succeed(localPublished, event.local); + return event.remote === null + ? Effect.void + : Deferred.succeed(remotePublished, event.remote); + }), + Effect.forkScoped, + ); + yield* Deferred.await(subscribed); + + const releaseRemote = yield* Deferred.make(); + state.beforeRemoteStatus = Deferred.await(releaseRemote); + state.currentLocalStatus = { ...baseLocalStatus, refName: "main" }; + state.currentRemoteStatus = remoteStatusWithPr; + yield* broadcaster.refreshAfterCheckout("/repo"); + + assert.equal((yield* Deferred.await(localPublished)).refName, "main"); + assert.isFalse(yield* Deferred.isDone(remotePublished)); + + yield* Deferred.succeed(releaseRemote, undefined); + assert.deepStrictEqual(yield* Deferred.await(remotePublished), remoteStatusWithPr); + assert.equal((yield* broadcaster.getStatus({ cwd: "/repo" })).pr?.number, 2978); + }).pipe(Effect.provide(layerTestFor(state)), Effect.scoped); + }); + + it.effect("a checkout's PR lookup is a user action that reuses cached lookups", () => { + const state: Parameters[0] = { + currentLocalStatus: baseLocalStatus, + currentRemoteStatus: baseRemoteStatus, + localStatusCalls: 0, + remoteStatusCalls: 0, + localInvalidationCalls: 0, + remoteInvalidationCalls: 0, + remoteStatusInteractiveValues: [], + invalidateStatusKeepLookupValues: [], + }; + + return Effect.gen(function* () { + const broadcaster = yield* VcsStatusBroadcaster.VcsStatusBroadcaster; + const remotePublished = yield* Deferred.make(); + const subscribed = yield* Deferred.make(); + yield* broadcaster.streamStatus({ cwd: "/repo", includeRemote: false }).pipe( + Stream.runForEach((event) => { + if (event._tag === "snapshot") return Deferred.succeed(subscribed, undefined); + return event._tag === "remoteUpdated" && event.remote !== null + ? Deferred.succeed(remotePublished, undefined) + : Effect.void; + }), + Effect.forkScoped, + ); + yield* Deferred.await(subscribed); + + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(remotePublished); + + assert.deepStrictEqual(state.remoteStatusInteractiveValues, [true]); + assert.deepStrictEqual(state.invalidateStatusKeepLookupValues, [true]); + }).pipe(Effect.provide(layerTestFor(state)), Effect.scoped); + }); + + it.effect("a checkout re-reads local status only when a fetch ran", () => { + const state: Parameters[0] = { + currentLocalStatus: baseLocalStatus, + currentRemoteStatus: baseRemoteStatus, + localStatusCalls: 0, + remoteStatusCalls: 0, + localInvalidationCalls: 0, + remoteInvalidationCalls: 0, + }; + + return Effect.gen(function* () { + const broadcaster = yield* VcsStatusBroadcaster.VcsStatusBroadcaster; + const subscribed = yield* Deferred.make(); + const firstRemote = yield* Deferred.make(); + const secondRemote = yield* Deferred.make(); + yield* broadcaster.streamStatus({ cwd: "/repo", includeRemote: false }).pipe( + Stream.runForEach((event) => { + if (event._tag === "snapshot") return Deferred.succeed(subscribed, undefined); + if (event._tag !== "remoteUpdated" || event.remote === null) return Effect.void; + return event.remote.pr === null + ? Deferred.succeed(firstRemote, undefined) + : Deferred.succeed(secondRemote, undefined); + }), + Effect.forkScoped, + ); + yield* Deferred.await(subscribed); + const readsBeforeCheckout = state.localStatusCalls; + + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(firstRemote); + assert.equal(state.localStatusCalls, readsBeforeCheckout + 1); + + const fetchedLocal: VcsStatusLocalResult = { + ...baseLocalStatus, + branchChanges: { baseRef: "origin/main", insertions: 0, deletions: 0 }, + }; + state.currentRemoteStatus = remoteStatusWithPr; + state.beforeRemoteStatus = Effect.sync(() => { + state.upstreamFetchCount = (state.upstreamFetchCount ?? 0) + 1; + state.currentLocalStatus = fetchedLocal; + }); + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(secondRemote); + + assert.equal(state.localStatusCalls, readsBeforeCheckout + 3); + assert.deepStrictEqual( + (yield* broadcaster.getStatus({ cwd: "/repo" })).branchChanges, + fetchedLocal.branchChanges, + ); + }).pipe(Effect.provide(layerTestFor(state)), Effect.scoped); + }); + it.effect("an explicit refresh reads local totals after the fetch", () => { const state: Parameters[0] = { currentLocalStatus: baseLocalStatus, diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.ts b/apps/server/src/vcs/VcsStatusBroadcaster.ts index a8839d0c1cd0c..de1d2b6fd8436 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.ts @@ -1,5 +1,6 @@ import * as Cause from "effect/Cause"; import * as Context from "effect/Context"; +import * as Deferred from "effect/Deferred"; import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as Exit from "effect/Exit"; @@ -23,6 +24,7 @@ import type { } from "@t3tools/contracts"; import { DEFAULT_AUTOMATIC_GIT_FETCH_INTERVAL } from "@t3tools/contracts"; import { resolveServerBackgroundActivitySettings } from "@t3tools/shared/backgroundActivitySettings"; +import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import { mergeGitStatusParts } from "@t3tools/shared/git"; import * as KeyedLock from "@t3tools/shared/KeyedLock"; import { resolveProjectSettings } from "@t3tools/shared/projectSettings"; @@ -211,6 +213,11 @@ export class VcsStatusBroadcaster extends Context.Service< readonly refreshLocalStatus: ( cwd: string, ) => Effect.Effect; + /** + * For a checkout someone is waiting on. Returns once the new branch is published, so the + * caller's reply does not outrun it, and finishes the slower remote read in the background. + */ + readonly refreshAfterCheckout: (cwd: string) => Effect.Effect; readonly refreshStatus: ( cwd: string, options?: { readonly localOnly?: boolean }, @@ -542,6 +549,48 @@ export const make = Effect.gen(function* () { ); }); + const refreshRemoteAfterCheckout = Effect.fn("VcsStatusBroadcaster.refreshRemoteAfterCheckout")( + function* (cwd: string, policyCwd: string) { + yield* withRemoteWriteLock( + cwd, + Effect.gen(function* () { + const refreshUpstream = !Duration.isZero(yield* automaticFetchInterval); + yield* workflow.invalidateStatus(cwd, { keepPullRequestLookups: true }); + const fetchesBefore = yield* workflow.statusUpstreamFetchCount(cwd); + const remoteRead = yield* readRemoteStatus(cwd, { refreshUpstream }); + // Only a fetch can move the base used for local Changes totals. + if ((yield* workflow.statusUpstreamFetchCount(cwd)) !== fetchesBefore) { + yield* refreshLocalStatusCore(cwd); + } + // Without a fetch, the cached behind count cannot justify an automatic pull. + const pulled = refreshUpstream + ? yield* maybeAutoPull(cwd, remoteRead, [policyCwd]) + : null; + if (pulled === null) { + yield* updateCachedRemoteStatus(cwd, remoteRead, { publish: true }); + } + }), + ); + }, + // Someone just picked this branch, so its PR lookup skips the background batching delay. + Effect.provideService(SourceControlRateLimit.Interactive, true), + ); + + const refreshAfterCheckout: VcsStatusBroadcaster["Service"]["refreshAfterCheckout"] = Effect.fn( + "VcsStatusBroadcaster.refreshAfterCheckout", + )(function* (rawCwd) { + const cwd = yield* withFileSystem(normalizeCwd(rawCwd)); + const localDone = yield* Deferred.make(); + // Owned by the broadcaster so a client disconnecting mid-switch still leaves status correct. + yield* refreshLocalStatusCore(cwd).pipe( + Effect.ensuring(Deferred.succeed(localDone, undefined)), + Effect.andThen(refreshRemoteAfterCheckout(cwd, rawCwd)), + Effect.ignoreCause({ log: true }), + Effect.forkIn(broadcasterScope), + ); + yield* Deferred.await(localDone); + }); + const refreshPullRequestStatus: VcsStatusBroadcaster["Service"]["refreshPullRequestStatus"] = Effect.fn("VcsStatusBroadcaster.refreshPullRequestStatus")(function* (rawCwd) { const cwd = yield* withFileSystem(normalizeCwd(rawCwd)); @@ -772,6 +821,7 @@ export const make = Effect.gen(function* () { return VcsStatusBroadcaster.of({ getStatus, refreshLocalStatus, + refreshAfterCheckout, refreshStatus, refreshPullRequestStatus, streamStatus, diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 8373bbfae0c55..44ba76497b686 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -3028,9 +3028,19 @@ export const layerWsRpc = ( [WS_METHODS.vcsRemoveWorktree]: (input) => gitWorkflow.removeWorktree(input).pipe(Effect.tap(() => refreshGitStatus(input.cwd))), [WS_METHODS.vcsCreateRef]: (input) => - gitWorkflow.createRef(input).pipe(Effect.tap(() => refreshGitStatus(input.cwd))), + gitWorkflow + .createRef(input) + .pipe( + Effect.tap(() => + input.switchRef + ? vcsStatusBroadcaster.refreshAfterCheckout(input.cwd) + : refreshGitStatus(input.cwd), + ), + ), [WS_METHODS.vcsSwitchRef]: (input) => - gitWorkflow.switchRef(input).pipe(Effect.tap(() => refreshGitStatus(input.cwd))), + gitWorkflow + .switchRef(input) + .pipe(Effect.tap(() => vcsStatusBroadcaster.refreshAfterCheckout(input.cwd))), [WS_METHODS.vcsInit]: (input) => vcsProvisioning.initRepository(input).pipe(Effect.tap(() => refreshGitStatus(input.cwd))), [WS_METHODS.reviewGetBranchCommits]: (input) => review.getBranchCommits(input), diff --git a/apps/web/src/components/BranchToolbar.logic.test.ts b/apps/web/src/components/BranchToolbar.logic.test.ts index 08e6f7633dee3..1b7f6887373ee 100644 --- a/apps/web/src/components/BranchToolbar.logic.test.ts +++ b/apps/web/src/components/BranchToolbar.logic.test.ts @@ -10,6 +10,7 @@ import { resolveLocalCheckoutBranchMismatch, sanitizeNewRefName, shouldIncludeBranchPickerItem, + isBranchHoldCurrent, } from "./BranchToolbar.logic"; describe("resolveDraftEnvModeAfterBranchChange", () => { @@ -235,6 +236,24 @@ describe("shouldIncludeBranchPickerItem", () => { }); }); +describe("isBranchHoldCurrent", () => { + const hold = { branch: "feature", replaces: "main", cwd: "/repo" }; + + it("keeps showing the checked-out branch while status still reports the old one", () => { + expect(isBranchHoldCurrent(hold, { canonicalBranch: "main", cwd: "/repo" })).toBe(true); + }); + + it("lets go once status moves, even to a different branch than the checkout", () => { + expect(isBranchHoldCurrent(hold, { canonicalBranch: "feature", cwd: "/repo" })).toBe(false); + expect(isBranchHoldCurrent(hold, { canonicalBranch: "other", cwd: "/repo" })).toBe(false); + expect(isBranchHoldCurrent(hold, { canonicalBranch: null, cwd: "/repo" })).toBe(false); + }); + + it("does not apply to another checkout that happens to be on the old branch", () => { + expect(isBranchHoldCurrent(hold, { canonicalBranch: "main", cwd: "/other" })).toBe(false); + }); +}); + describe("sanitizeNewRefName", () => { it("collapses a run of whitespace into a single dash", () => { expect(sanitizeNewRefName("new branch")).toBe("new-branch"); diff --git a/apps/web/src/components/BranchToolbar.logic.ts b/apps/web/src/components/BranchToolbar.logic.ts index e22dcfaddedeb..4895c7f34ea7c 100644 --- a/apps/web/src/components/BranchToolbar.logic.ts +++ b/apps/web/src/components/BranchToolbar.logic.ts @@ -196,6 +196,23 @@ export function resolveBranchToolbarValue(input: { return currentGitBranch ?? activeThreadBranch; } +export interface BranchHold { + readonly branch: string | null; + readonly replaces: string | null; + readonly cwd: string; +} + +/** + * A finished checkout's branch stays on screen until status reports any change, because the status + * stream and the RPC reply are not ordered and showing status first would flip back to the old branch. + */ +export function isBranchHoldCurrent( + hold: BranchHold, + current: { readonly canonicalBranch: string | null; readonly cwd: string | null }, +): boolean { + return hold.cwd === current.cwd && hold.replaces === current.canonicalBranch; +} + export function resolveBranchTriggerLabel(input: { activeWorktreePath: string | null; effectiveEnvMode: EnvMode; diff --git a/apps/web/src/components/BranchToolbarBranchSelector.tsx b/apps/web/src/components/BranchToolbarBranchSelector.tsx index 8f6ffdf0a119e..129f94e2d38cf 100644 --- a/apps/web/src/components/BranchToolbarBranchSelector.tsx +++ b/apps/web/src/components/BranchToolbarBranchSelector.tsx @@ -46,7 +46,9 @@ import { parsePullRequestReference } from "@t3tools/client-runtime/pull-request- import { getSourceControlPresentation } from "../sourceControlPresentation"; import { useComposerMenuProps } from "./chat/composerEventScope"; import { + type BranchHold, deriveLocalBranchNameFromRemoteRef, + isBranchHoldCurrent, resolveBranchTriggerLabel, resolveBranchToolbarPrBranch, resolveBranchSelectionTarget, @@ -286,8 +288,13 @@ export function BranchToolbarBranchSelector({ activeThreadBranch, currentGitBranch, }); + const [branchHold, setBranchHold] = useState(null); + const branchHoldCurrent = + branchHold !== null && + isBranchHoldCurrent(branchHold, { canonicalBranch: canonicalActiveBranch, cwd: branchCwd }); + if (branchHold !== null && !branchHoldCurrent) setBranchHold(null); const [resolvedActiveBranch, setOptimisticBranch] = useOptimistic( - canonicalActiveBranch, + branchHoldCurrent ? branchHold.branch : canonicalActiveBranch, (_currentBranch: string | null, optimisticBranch: string | null) => optimisticBranch, ); const listedActiveBranch = @@ -376,7 +383,6 @@ export function BranchToolbarBranchSelector({ startBranchActionTransition(async () => { await action(); branchRefState.refresh(); - branchStatusQuery.refresh(); }); }; @@ -425,6 +431,11 @@ export function BranchToolbarBranchSelector({ ? (checkoutResult.value.refName ?? selectedBranchName) : selectedBranchName; setOptimisticBranch(nextBranchName); + setBranchHold({ + branch: nextBranchName, + replaces: canonicalActiveBranch, + cwd: selectionTarget.checkoutCwd, + }); setThreadBranch(nextBranchName, selectionTarget.nextWorktreePath); return; } @@ -462,6 +473,11 @@ export function BranchToolbarBranchSelector({ }); if (createBranchResult._tag === "Success") { setOptimisticBranch(createBranchResult.value.refName); + setBranchHold({ + branch: createBranchResult.value.refName, + replaces: canonicalActiveBranch, + cwd: branchCwd, + }); setThreadBranch(createBranchResult.value.refName, activeWorktreePath); return; }