diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index a71f19e6a973b..34132bc844c7e 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -969,7 +969,10 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { }), ); - it.effect("a git read in flight during a T3 git action does not outlive it", () => + it.effect.each([ + { action: "a T3 git action", options: undefined }, + { action: "a checkout", options: { keepPullRequestLookups: true } }, + ])("a git read in flight during $action does not outlive it", ({ options }) => Effect.gen(function* () { const repoDir = yield* makeTempDir("t3code-git-manager-"); yield* initRepo(repoDir); @@ -1012,7 +1015,7 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { const inFlight = yield* Effect.forkChild(lookup); yield* Deferred.await(gate.reached); yield* runGit(repoDir, ["branch", "--set-upstream-to=origin/feature/raced-head"]); - yield* manager.invalidateStatus(repoDir); + yield* manager.invalidateStatus(repoDir, options); yield* Deferred.succeed(gate.release, undefined); yield* Fiber.join(inFlight); @@ -2433,6 +2436,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..396bcc430252d 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; @@ -1151,6 +1155,15 @@ export const make = Effect.gen(function* () { prLookupEpochByCwd.set(cacheKey, prLookupEpoch(cacheKey) + 1); }), ); + // Separate from the PR lookup epoch because a checkout keeps PR lookups but changes git state. + const prLookupGitStateEpochByCwd = new Map(); + const prLookupGitStateEpoch = (cwd: string) => prLookupGitStateEpochByCwd.get(cwd) ?? 0; + const bumpPrLookupGitStateEpoch = (cwd: string) => + normalizeStatusCacheKey(cwd).pipe( + Effect.map((cacheKey) => { + prLookupGitStateEpochByCwd.set(cacheKey, prLookupGitStateEpoch(cacheKey) + 1); + }), + ); const prLookupCacheKey = ( cwd: string, details: { @@ -2403,10 +2416,13 @@ export const make = Effect.gen(function* () { } // invalidateStatus bumps the epoch. A read that started before a T3 git // action, or before a newer read finished, must not replace their state. - const epoch = prLookupEpoch(cwd); + const epoch = prLookupGitStateEpoch(cwd); const value = yield* read; const latest = prLookupGitStateByKey.get(entryKey); - if (prLookupEpoch(cwd) !== epoch || (latest !== undefined && latest.readAtMs > readAtMs)) { + if ( + prLookupGitStateEpoch(cwd) !== epoch || + (latest !== undefined && latest.readAtMs > readAtMs) + ) { return value; } prLookupGitStateByKey.delete(entryKey); @@ -2665,10 +2681,11 @@ 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* bumpPrLookupGitStateEpoch(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..c9e71c222c24b 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -1,6 +1,8 @@ // @effect-diagnostics nodeBuiltinImport:off - realpathSync.native resolves Windows 8.3 short names, which the Effect realPath does not. +import * as NodeChildProcess from "node:child_process"; import * as NodeFS from "node:fs"; import * as NodeOS from "node:os"; +import * as NodePath from "node:path"; import * as NodeURL from "node:url"; import * as NodeServices from "@effect/platform-node/NodeServices"; import * as HostProcess from "@t3tools/shared/HostProcess"; @@ -3386,6 +3388,68 @@ 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); + }), + ); + + // Needs mkfifo to hold the fetch open at a known point. + it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( + "does not count a status fetch that is still running after its wait times out", + () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const remote = yield* makeTmpDir("git-vcs-driver-remote-"); + const control = yield* makeTmpDir("git-vcs-driver-control-"); + 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 started = NodePath.join(control, "started"); + const release = NodePath.join(control, "release"); + NodeChildProcess.execFileSync("mkfifo", [started, release]); + const uploadPack = NodePath.join(control, "upload-pack.sh"); + NodeFS.writeFileSync( + uploadPack, + `#!/bin/sh\necho started > "${started}"\ncat "${release}" > /dev/null\nexec git-upload-pack "$@"\n`, + { mode: 0o755 }, + ); + // Git runs this setting through a shell, and the temp path can contain spaces. + yield* git(cwd, ["config", "remote.origin.uploadpack", `'${uploadPack}'`]); + + const driver = yield* GitVcsDriver.GitVcsDriver; + const initial = yield* driver.statusUpstreamFetchCount(cwd); + const status = yield* driver.statusDetailsRemote(cwd).pipe(Effect.forkScoped); + yield* Effect.promise(() => NodeFS.promises.readFile(started)); + yield* TestClock.adjust("6 seconds"); + yield* Fiber.join(status); + + assert.equal(yield* driver.statusUpstreamFetchCount(cwd), initial); + yield* Effect.promise(() => NodeFS.promises.writeFile(release, "go")); + }), + ); + 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..e89c885d87e58 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -1326,6 +1326,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ); }); + const statusUpstreamFetches = new Map(); const fetchRemoteForStatus = Effect.fn("GitVcsDriver.fetchRemoteForStatus.wait")(function* ( gitCommonDir: string, remoteName: string, @@ -1374,6 +1375,12 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* Effect.onExit(() => Effect.sync(() => { statusFetches.delete(key); + // Counted at exit, not when a waiter stops waiting: a waiter can time out before refs + // move. Failures count too, because a failed fetch can still move some refs. + statusUpstreamFetches.set( + gitCommonDir, + (statusUpstreamFetches.get(gitCommonDir) ?? 0) + 1, + ); }), ), Effect.forkIn(serviceScope, { startImmediately: false, uninterruptible: false }), @@ -1640,6 +1647,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 +4950,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 be3a286ffc32b..c1ec6dad69afc 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,161 @@ 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(); + const thirdRemote = 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; + if (event.remote.pr === null) return Deferred.succeed(firstRemote, undefined); + return event.remote.pr.number === 2979 + ? Deferred.succeed(thirdRemote, 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, + ); + + // A fetch finishing after the first local read took its values still needs a re-read. + const lateFetchLocal: VcsStatusLocalResult = { + ...baseLocalStatus, + branchChanges: { baseRef: "origin/main", insertions: 7, deletions: 0 }, + }; + state.beforeRemoteStatus = Effect.void; + state.currentRemoteStatus = { + ...remoteStatusWithPr, + pr: remoteStatusWithPr.pr && { ...remoteStatusWithPr.pr, number: 2979 }, + }; + state.beforeLocalStatus = Effect.sync(() => { + state.beforeLocalStatus = Effect.void; + state.upstreamFetchCount = (state.upstreamFetchCount ?? 0) + 1; + state.currentLocalStatus = lateFetchLocal; + }); + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(thirdRemote); + + assert.deepStrictEqual( + (yield* broadcaster.getStatus({ cwd: "/repo" })).branchChanges, + lateFetchLocal.branchChanges, + ); + }).pipe(Effect.provide(layerTestFor(state)), Effect.scoped); + }); + it.effect("a remote poll refreshes Changes totals when divergence counts stay the same", () => { const state: Parameters[0] = { currentLocalStatus: { diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.ts b/apps/server/src/vcs/VcsStatusBroadcaster.ts index 327e0bcb1bb41..f3d8018a56cba 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 }, @@ -490,6 +497,30 @@ export const make = Effect.gen(function* () { ); }); + const refreshRemoteAndLocalTotals = Effect.fn("VcsStatusBroadcaster.refreshRemoteAndLocalTotals")( + function* (cwd: string, policyCwd: string) { + return yield* withRemoteWriteLock( + cwd, + Effect.gen(function* () { + const refreshUpstream = !Duration.isZero(yield* automaticFetchInterval); + // Bump the PR lookup epoch after earlier remote writers have finished. + yield* workflow.invalidateStatus(cwd); + const remoteRead = yield* readRemoteStatus(cwd, { refreshUpstream }); + // Fetch can move the base used for local Changes totals. + 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) return mergeGitStatusParts(pulled.local, pulled.remote); + yield* updateCachedRemoteStatus(cwd, remoteRead, { publish: true }); + const parts = yield* getStatusParts(cwd); + return mergeGitStatusParts(parts.local, parts.remote); + }), + ); + }, + ); + const refreshStatus: VcsStatusBroadcaster["Service"]["refreshStatus"] = Effect.fn( "VcsStatusBroadcaster.refreshStatus", )(function* (rawCwd, options) { @@ -513,23 +544,50 @@ export const make = Effect.gen(function* () { }), ); } - return yield* withRemoteWriteLock( - cwd, - Effect.gen(function* () { - const refreshUpstream = !Duration.isZero(yield* automaticFetchInterval); - // Bump the PR lookup epoch after earlier remote writers have finished. - yield* workflow.invalidateStatus(cwd); - const remoteRead = yield* readRemoteStatus(cwd, { refreshUpstream }); - // Fetch can move the base used for local Changes totals. - yield* refreshLocalStatusCore(cwd); - // Without a fetch, the cached behind count cannot justify an automatic pull. - const pulled = refreshUpstream ? yield* maybeAutoPull(cwd, remoteRead, [rawCwd]) : null; - if (pulled !== null) return mergeGitStatusParts(pulled.local, pulled.remote); - yield* updateCachedRemoteStatus(cwd, remoteRead, { publish: true }); - const parts = yield* getStatusParts(cwd); - return mergeGitStatusParts(parts.local, parts.remote); - }), + return yield* refreshRemoteAndLocalTotals(cwd, rawCwd); + }); + + const refreshRemoteAfterCheckout = Effect.fn("VcsStatusBroadcaster.refreshRemoteAfterCheckout")( + function* (cwd: string, policyCwd: string, fetchesBefore: number) { + yield* withRemoteWriteLock( + cwd, + Effect.gen(function* () { + const refreshUpstream = !Duration.isZero(yield* automaticFetchInterval); + yield* workflow.invalidateStatus(cwd, { keepPullRequestLookups: true }); + 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)); + // Taken before the local read, so a fetch that finishes after it still triggers a re-read. + const fetchesBefore = yield* workflow.statusUpstreamFetchCount(cwd); + 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, fetchesBefore)), + Effect.ignoreCause({ log: true }), + Effect.forkIn(broadcasterScope), ); + yield* Deferred.await(localDone); }); const refreshPullRequestStatus: VcsStatusBroadcaster["Service"]["refreshPullRequestStatus"] = @@ -762,6 +820,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 abc7c71f4b8b2..5be83f169cf07 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -3048,9 +3048,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..6819e8242a3cd 100644 --- a/apps/web/src/components/BranchToolbar.logic.test.ts +++ b/apps/web/src/components/BranchToolbar.logic.test.ts @@ -1,4 +1,4 @@ -import { type VcsRef } from "@t3tools/contracts"; +import { EnvironmentId, type VcsRef } from "@t3tools/contracts"; import { describe, expect, it } from "vite-plus/test"; import { dedupeRemoteBranchesWithLocalMatches } from "@t3tools/shared/git"; import { @@ -10,6 +10,7 @@ import { resolveLocalCheckoutBranchMismatch, sanitizeNewRefName, shouldIncludeBranchPickerItem, + isBranchHoldCurrent, } from "./BranchToolbar.logic"; describe("resolveDraftEnvModeAfterBranchChange", () => { @@ -235,6 +236,32 @@ describe("shouldIncludeBranchPickerItem", () => { }); }); +describe("isBranchHoldCurrent", () => { + const environmentId = EnvironmentId.make("env-a"); + const hold = { branch: "feature", replaces: "main", environmentId, cwd: "/repo" }; + const current = { canonicalBranch: "main", environmentId, cwd: "/repo" }; + + it("keeps showing the checked-out branch while status still reports the old one", () => { + expect(isBranchHoldCurrent(hold, current)).toBe(true); + }); + + it("lets go once status moves, even to a different branch than the checkout", () => { + for (const canonicalBranch of ["feature", "other", null]) { + expect(isBranchHoldCurrent(hold, { ...current, canonicalBranch })).toBe(false); + } + }); + + it("does not apply to another checkout that happens to be on the old branch", () => { + expect(isBranchHoldCurrent(hold, { ...current, cwd: "/other" })).toBe(false); + }); + + it("does not apply to the same path on another environment", () => { + expect( + isBranchHoldCurrent(hold, { ...current, environmentId: EnvironmentId.make("env-b") }), + ).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..e882daa386a9f 100644 --- a/apps/web/src/components/BranchToolbar.logic.ts +++ b/apps/web/src/components/BranchToolbar.logic.ts @@ -196,6 +196,32 @@ export function resolveBranchToolbarValue(input: { return currentGitBranch ?? activeThreadBranch; } +export interface BranchHold { + readonly branch: string | null; + readonly replaces: string | null; + readonly environmentId: EnvironmentId; + 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 environmentId: EnvironmentId; + readonly cwd: string | null; + }, +): boolean { + return ( + hold.environmentId === current.environmentId && + 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..80ff1970888ef 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,17 @@ export function BranchToolbarBranchSelector({ activeThreadBranch, currentGitBranch, }); + const [branchHold, setBranchHold] = useState(null); + const branchHoldCurrent = + branchHold !== null && + isBranchHoldCurrent(branchHold, { + canonicalBranch: canonicalActiveBranch, + environmentId, + 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 +387,6 @@ export function BranchToolbarBranchSelector({ startBranchActionTransition(async () => { await action(); branchRefState.refresh(); - branchStatusQuery.refresh(); }); }; @@ -425,6 +435,12 @@ export function BranchToolbarBranchSelector({ ? (checkoutResult.value.refName ?? selectedBranchName) : selectedBranchName; setOptimisticBranch(nextBranchName); + setBranchHold({ + branch: nextBranchName, + replaces: canonicalActiveBranch, + environmentId, + cwd: selectionTarget.checkoutCwd, + }); setThreadBranch(nextBranchName, selectionTarget.nextWorktreePath); return; } @@ -462,6 +478,12 @@ export function BranchToolbarBranchSelector({ }); if (createBranchResult._tag === "Success") { setOptimisticBranch(createBranchResult.value.refName); + setBranchHold({ + branch: createBranchResult.value.refName, + replaces: canonicalActiveBranch, + environmentId, + cwd: branchCwd, + }); setThreadBranch(createBranchResult.value.refName, activeWorktreePath); return; }