diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts new file mode 100644 index 00000000000..3dd93a28882 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts @@ -0,0 +1,247 @@ +import { describe, expect, mock, test } from "bun:test"; + +/** + * Tests for getGitHubStatus procedure logic. + * + * Reproduces #2592: branch workspaces (no worktreeId) returned null + * from getGitHubStatus because the procedure required a worktree record. + */ + +const mockGetWorkspace = mock(); +const mockGetWorktree = mock(); +const mockGetProject = mock(); +const mockGetWorkspacePath = mock(); +const mockFetchGitHubPRStatus = mock(); +const mockFetchGitHubPRComments = mock(); +const mockUpdateProjectDefaultBranch = mock(); +const mockLocalDb = { + update: mock(() => ({ + set: mock(() => ({ + where: mock(() => ({ + run: mock(), + })), + })), + })), + select: mock(() => ({ + from: mock(() => ({ + where: mock(() => ({ + all: mock(() => []), + get: mock(() => undefined), + })), + })), + })), +}; + +// Mock @superset/local-db +mock.module("@superset/local-db", () => ({ + workspaces: { id: "id" }, + worktrees: { id: "id", projectId: "project_id" }, +})); + +mock.module("drizzle-orm", () => ({ + eq: mock(() => "eq"), + and: mock((...args: unknown[]) => args), + isNull: mock(() => "isNull"), +})); + +mock.module("../utils/db-helpers", () => ({ + getWorkspace: mockGetWorkspace, + getWorktree: mockGetWorktree, + getProject: mockGetProject, + updateProjectDefaultBranch: mockUpdateProjectDefaultBranch, +})); + +mock.module("../utils/worktree", () => ({ + getWorkspacePath: mockGetWorkspacePath, +})); + +mock.module("../utils/github", () => ({ + fetchGitHubPRComments: mockFetchGitHubPRComments, + fetchGitHubPRStatus: mockFetchGitHubPRStatus, + clearGitHubCachesForWorktree: mock(), + resolveReviewThread: mock(), +})); + +mock.module("main/lib/local-db", () => ({ + localDb: mockLocalDb, +})); + +mock.module("../../..", () => ({ + publicProcedure: { + input: () => ({ + query: (fn: unknown) => fn, + mutation: (fn: unknown) => fn, + }), + }, + router: (routes: Record) => routes, +})); + +mock.module("../utils/git", () => ({ + branchExistsOnRemote: mock(() => ({ status: "missing" })), + fetchDefaultBranch: mock(), + getAheadBehindCount: mock(() => ({ ahead: 0, behind: 0 })), + getDefaultBranch: mock(() => "main"), + listExternalWorktrees: mock(() => []), + refreshDefaultBranch: mock(() => "main"), +})); + +mock.module("node:fs", () => ({ + existsSync: mock(() => true), +})); + +const { createGitStatusProcedures } = await import("./git-status"); + +const procedures = createGitStatusProcedures() as unknown as Record< + string, + (opts: { input: Record }) => Promise +>; + +const fakeGitHubStatus = { + pr: { number: 42, title: "Test PR", url: "https://github.com/test/pr/42" }, + repoUrl: "https://github.com/test/repo", + upstreamUrl: "https://github.com/test/repo", + isFork: false, + branchExistsOnRemote: true, + previewUrl: undefined, + lastRefreshed: Date.now(), +}; + +describe("getGitHubStatus", () => { + test("returns GitHub status for branch workspaces (no worktreeId)", async () => { + const branchWorkspace = { + id: "ws-branch-1", + projectId: "proj-1", + worktreeId: null, + type: "branch" as const, + branch: "feature/branch-workspace", + name: "Feature Branch", + }; + + mockFetchGitHubPRStatus.mockClear(); + mockGetWorktree.mockClear(); + mockGetWorkspace.mockReturnValue(branchWorkspace); + mockGetWorkspacePath.mockReturnValue("/repos/my-project"); + mockFetchGitHubPRStatus.mockResolvedValue(fakeGitHubStatus); + + const result = await procedures.getGitHubStatus({ + input: { workspaceId: "ws-branch-1" }, + }); + + expect(result).not.toBeNull(); + expect(mockGetWorkspacePath).toHaveBeenCalledWith(branchWorkspace); + expect(mockFetchGitHubPRStatus).toHaveBeenCalledWith( + "/repos/my-project", + "feature/branch-workspace", + ); + expect(mockGetWorktree).not.toHaveBeenCalled(); + }); + + test("returns GitHub status for worktree workspaces", async () => { + const worktreeWorkspace = { + id: "ws-wt-1", + projectId: "proj-1", + worktreeId: "wt-1", + type: "worktree" as const, + branch: "feature/foo", + name: "Feature Foo", + }; + + mockFetchGitHubPRStatus.mockClear(); + mockGetWorkspace.mockReturnValue(worktreeWorkspace); + mockGetWorktree.mockReturnValue({ + id: "wt-1", + path: "/repos/my-project/.worktrees/feature-foo", + branch: "feature/foo", + githubStatus: null, + }); + mockGetWorkspacePath.mockReturnValue( + "/repos/my-project/.worktrees/feature-foo", + ); + mockFetchGitHubPRStatus.mockResolvedValue(fakeGitHubStatus); + + const result = await procedures.getGitHubStatus({ + input: { workspaceId: "ws-wt-1" }, + }); + + expect(result).not.toBeNull(); + expect(mockFetchGitHubPRStatus).toHaveBeenCalledWith( + "/repos/my-project/.worktrees/feature-foo", + null, + ); + }); + + test("returns null when workspace not found", async () => { + mockGetWorkspace.mockReturnValue(undefined); + + const result = await procedures.getGitHubStatus({ + input: { workspaceId: "nonexistent" }, + }); + + expect(result).toBeNull(); + }); + + test("returns null when workspace path cannot be resolved", async () => { + mockGetWorkspace.mockReturnValue({ + id: "ws-1", + projectId: "proj-1", + worktreeId: null, + type: "branch", + }); + mockGetWorkspacePath.mockReturnValue(null); + + const result = await procedures.getGitHubStatus({ + input: { workspaceId: "ws-1" }, + }); + + expect(result).toBeNull(); + }); + + test("does not cache to worktrees table for branch workspaces", async () => { + const branchWorkspace = { + id: "ws-branch-2", + projectId: "proj-1", + worktreeId: null, + type: "branch" as const, + branch: "main", + }; + + mockGetWorkspace.mockReturnValue(branchWorkspace); + mockGetWorkspacePath.mockReturnValue("/repos/my-project"); + mockFetchGitHubPRStatus.mockResolvedValue(fakeGitHubStatus); + mockLocalDb.update.mockClear(); + + await procedures.getGitHubStatus({ + input: { workspaceId: "ws-branch-2" }, + }); + + expect(mockLocalDb.update).not.toHaveBeenCalled(); + }); +}); + +describe("getWorktreeInfo", () => { + test("returns info for branch workspaces", async () => { + const branchWorkspace = { + id: "ws-branch-3", + projectId: "proj-1", + worktreeId: null, + type: "branch" as const, + branch: "feature/my-branch", + name: "My Branch Workspace", + createdAt: 1234567890, + }; + + mockGetWorkspace.mockReturnValue(branchWorkspace); + + const result = await procedures.getWorktreeInfo({ + input: { workspaceId: "ws-branch-3" }, + }); + + expect(result).toEqual({ + worktreeName: "My Branch Workspace", + branchName: "feature/my-branch", + createdAt: 1234567890, + gitStatus: null, + githubStatus: null, + }); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts index 91ec39d7137..c3229e1dc1e 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts @@ -25,6 +25,7 @@ import { type PullRequestCommentsTarget, resolveReviewThread, } from "../utils/github"; +import { getWorkspacePath } from "../utils/worktree"; const gitHubPRCommentsInputSchema = z.object({ workspaceId: z.string(), @@ -101,12 +102,10 @@ export const createGitStatusProcedures = () => { throw new Error(`Workspace ${input.workspaceId} not found`); } - const worktree = workspace.worktreeId - ? getWorktree(workspace.worktreeId) - : null; - if (!worktree) { + const repoPath = getWorkspacePath(workspace); + if (!repoPath) { throw new Error( - `Worktree for workspace ${input.workspaceId} not found`, + `Could not resolve path for workspace ${input.workspaceId}`, ); } @@ -134,23 +133,25 @@ export const createGitStatusProcedures = () => { await fetchDefaultBranch(project.mainRepoPath, defaultBranch); const { ahead, behind } = await getAheadBehindCount({ - repoPath: worktree.path, + repoPath, defaultBranch, }); const gitStatus = { - branch: worktree.branch, + branch: workspace.branch, needsRebase: behind > 0, ahead, behind, lastRefreshed: Date.now(), }; - localDb - .update(worktrees) - .set({ gitStatus }) - .where(eq(worktrees.id, worktree.id)) - .run(); + if (workspace.worktreeId) { + localDb + .update(worktrees) + .set({ gitStatus }) + .where(eq(worktrees.id, workspace.worktreeId)) + .run(); + } return { gitStatus, defaultBranch }; }), @@ -182,27 +183,31 @@ export const createGitStatusProcedures = () => { return null; } - const worktree = workspace.worktreeId - ? getWorktree(workspace.worktreeId) - : null; - if (!worktree) { + const repoPath = getWorkspacePath(workspace); + if (!repoPath) { return null; } - const freshStatus = await fetchGitHubPRStatus(worktree.path); - - if ( - freshStatus && - hasMeaningfulGitHubStatusChange({ - current: worktree.githubStatus, - next: freshStatus, - }) - ) { - localDb - .update(worktrees) - .set({ githubStatus: freshStatus }) - .where(eq(worktrees.id, worktree.id)) - .run(); + const branchOverride = + workspace.type === "branch" ? workspace.branch : null; + + const freshStatus = await fetchGitHubPRStatus(repoPath, branchOverride); + + if (freshStatus && workspace.worktreeId) { + const worktree = getWorktree(workspace.worktreeId); + if ( + worktree && + hasMeaningfulGitHubStatusChange({ + current: worktree.githubStatus, + next: freshStatus, + }) + ) { + localDb + .update(worktrees) + .set({ githubStatus: freshStatus }) + .where(eq(worktrees.id, workspace.worktreeId)) + .run(); + } } return freshStatus; @@ -216,21 +221,23 @@ export const createGitStatusProcedures = () => { return []; } - const worktree = workspace.worktreeId - ? getWorktree(workspace.worktreeId) - : null; - if (!worktree) { + const repoPath = getWorkspacePath(workspace); + if (!repoPath) { return []; } - const cachedGitHubStatus = worktree.githubStatus ?? null; + const worktree = workspace.worktreeId + ? getWorktree(workspace.worktreeId) + : null; + const cachedGitHubStatus = worktree?.githubStatus ?? null; return fetchGitHubPRComments({ - worktreePath: worktree.path, + worktreePath: repoPath, pullRequest: resolveCommentsPullRequestTarget({ input, githubStatus: cachedGitHubStatus, }), + branchName: workspace.type === "branch" ? workspace.branch : null, }); }), @@ -248,22 +255,20 @@ export const createGitStatusProcedures = () => { throw new Error(`Workspace ${input.workspaceId} not found`); } - const worktree = workspace.worktreeId - ? getWorktree(workspace.worktreeId) - : null; - if (!worktree) { + const repoPath = getWorkspacePath(workspace); + if (!repoPath) { throw new Error( - `Worktree for workspace ${input.workspaceId} not found`, + `Could not resolve path for workspace ${input.workspaceId}`, ); } await resolveReviewThread({ - worktreePath: worktree.path, + worktreePath: repoPath, threadId: input.threadId, resolve: input.resolve, }); - clearGitHubCachesForWorktree(worktree.path); + clearGitHubCachesForWorktree(repoPath); }), getWorktreeInfo: publicProcedure @@ -274,6 +279,16 @@ export const createGitStatusProcedures = () => { return null; } + if (workspace.type === "branch") { + return { + worktreeName: workspace.name, + branchName: workspace.branch, + createdAt: workspace.createdAt, + gitStatus: null, + githubStatus: null, + }; + } + const worktree = workspace.worktreeId ? getWorktree(workspace.worktreeId) : null; diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts index adda1913d44..f1712477e25 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/cache.ts @@ -133,7 +133,7 @@ export function readCachedRepoContext( } export function clearGitHubCachesForWorktree(worktreePath: string): void { - githubStatusResource.invalidate(worktreePath); + githubStatusResource.invalidatePrefix(worktreePath); repoContextResource.invalidate(worktreePath); pullRequestCommentsResource.invalidatePrefix( makePullRequestCommentsCachePrefix(worktreePath), diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts index 66ff14f3cf1..e4291c632a2 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts @@ -42,25 +42,36 @@ function getPullRequestCommentsRepoNameWithOwner( async function resolvePullRequestCommentsTarget( worktreePath: string, + branchOverride?: string | null, ): Promise { const repoContext = await getRepoContext(worktreePath); if (!repoContext) { return null; } - const branchName = await getCurrentBranch(worktreePath); + const branchName = + branchOverride?.trim() || (await getCurrentBranch(worktreePath)); if (!branchName) { return null; } - const shaResult = await execGitWithShellPath(["rev-parse", "HEAD"], { + + const revParseTarget = branchOverride ? `refs/heads/${branchName}` : "HEAD"; + const shaResult = await execGitWithShellPath(["rev-parse", revParseTarget], { cwd: worktreePath, }).catch((error) => { if (isUnbornHeadError(error)) { return { stdout: "", stderr: "" }; } + if (branchOverride) { + return { stdout: "", stderr: "" }; + } throw error; }); const headSha = shaResult.stdout.trim() || undefined; + + if (branchOverride && !headSha) { + return null; + } const prInfo = await getPRForBranch( worktreePath, branchName, @@ -91,6 +102,7 @@ export function resolveRemoteBranchNameForGitHubStatus({ async function refreshGitHubPRStatus( worktreePath: string, + branchOverride?: string | null, ): Promise { try { const repoContext = await getRepoContext(worktreePath); @@ -98,25 +110,41 @@ async function refreshGitHubPRStatus( return null; } - const branchName = await getCurrentBranch(worktreePath); + const branchName = + branchOverride?.trim() || (await getCurrentBranch(worktreePath)); if (!branchName) { return null; } + const revParseTarget = branchOverride ? `refs/heads/${branchName}` : "HEAD"; + const upstreamTarget = branchOverride + ? `${branchName}@{upstream}` + : "@{upstream}"; + const [shaResult, upstreamResult] = await Promise.all([ - execGitWithShellPath(["rev-parse", "HEAD"], { + execGitWithShellPath(["rev-parse", revParseTarget], { cwd: worktreePath, }).catch((error) => { if (isUnbornHeadError(error)) { return { stdout: "", stderr: "" }; } + if (branchOverride) { + return { stdout: "", stderr: "" }; + } throw error; }), - execGitWithShellPath(["rev-parse", "--abbrev-ref", "@{upstream}"], { + execGitWithShellPath(["rev-parse", "--abbrev-ref", upstreamTarget], { cwd: worktreePath, }).catch(() => ({ stdout: "", stderr: "" })), ]); const headSha = shaResult.stdout.trim() || undefined; + + // When using a branch override, we must have a valid SHA to avoid + // getPRForBranch falling back to HEAD (which is a different branch). + if (branchOverride && !headSha) { + return null; + } + const parsedUpstreamRef = parseUpstreamRef(upstreamResult.stdout.trim()); const trackingRemote = parsedUpstreamRef?.remoteName ?? "origin"; const previewBranchName = resolveRemoteBranchNameForGitHubStatus({ @@ -194,27 +222,36 @@ async function refreshGitHubPRComments({ } /** - * Fetches GitHub PR status for a worktree using the `gh` CLI. + * Fetches GitHub PR status for a worktree or branch workspace using the `gh` CLI. * Returns null if `gh` is not installed, not authenticated, or on error. + * + * @param branchName - Optional branch name override. When provided (for branch + * workspaces), resolves the SHA and upstream for that branch instead of using + * HEAD / the checked-out branch. Also used to scope the cache key. */ export async function fetchGitHubPRStatus( worktreePath: string, + branchName?: string | null, ): Promise { - return readCachedGitHubStatus(worktreePath, () => - refreshGitHubPRStatus(worktreePath), + const cacheKey = branchName ? `${worktreePath}::${branchName}` : worktreePath; + return readCachedGitHubStatus(cacheKey, () => + refreshGitHubPRStatus(worktreePath, branchName), ); } export async function fetchGitHubPRComments({ worktreePath, pullRequest, + branchName, }: { worktreePath: string; pullRequest?: PullRequestCommentsTarget | null; + branchName?: string | null; }): Promise { try { const pullRequestTarget = - pullRequest ?? (await resolvePullRequestCommentsTarget(worktreePath)); + pullRequest ?? + (await resolvePullRequestCommentsTarget(worktreePath, branchName)); if (!pullRequestTarget) { return []; }