From dc36a1c8405d6f5cbbe2a972f4315d19e1690bff Mon Sep 17 00:00:00 2001 From: Tom Whittle Date: Thu, 9 Apr 2026 15:34:41 +0100 Subject: [PATCH 1/3] fix(desktop): resolve GitHub status for branch workspaces Use getWorkspacePath() instead of requiring a worktree record so branch-based workspaces can access GitHub PR status, comments, and review thread resolution. Pass the workspace branch name explicitly to fetchGitHubPRStatus so the correct branch is looked up even when the repo is checked out to a different branch. Bail out early when a branch override's SHA cannot be resolved to prevent getPRForBranch from falling back to HEAD and matching the wrong PR. Closes #2592 --- .../workspaces/procedures/git-status.test.ts | 245 ++++++++++++++++++ .../workspaces/procedures/git-status.ts | 100 ++++--- .../routers/workspaces/utils/github/github.ts | 36 ++- 3 files changed, 330 insertions(+), 51 deletions(-) create mode 100644 apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts 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..851c398788c --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.test.ts @@ -0,0 +1,245 @@ +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(); + 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", + ); + }); + + 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..6ba10d7dfa8 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,17 +221,18 @@ 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, @@ -248,22 +254,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 +278,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/github.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts index 66ff14f3cf1..d884ed95677 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 @@ -91,6 +91,7 @@ export function resolveRemoteBranchNameForGitHubStatus({ async function refreshGitHubPRStatus( worktreePath: string, + branchOverride?: string | null, ): Promise { try { const repoContext = await getRepoContext(worktreePath); @@ -98,25 +99,38 @@ 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)) { + if (!branchOverride && isUnbornHeadError(error)) { return { stdout: "", stderr: "" }; } - throw error; + return { stdout: "", stderr: "" }; }), - 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,14 +208,20 @@ 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), ); } From 1daa4dbf0aeccba99f1a01a4e0f9eeaef35c9359 Mon Sep 17 00:00:00 2001 From: Tom Whittle Date: Thu, 9 Apr 2026 15:54:24 +0100 Subject: [PATCH 2/3] fix(desktop): address PR review feedback - Use invalidatePrefix for GitHub status cache so branch-scoped keys (path::branch) are properly cleared on invalidation - Restore re-throw for unexpected git rev-parse errors on the non-override (worktree) path instead of swallowing all errors - Pass branch override to resolvePullRequestCommentsTarget so PR comments resolve from the workspace branch, not HEAD --- .../workspaces/procedures/git-status.ts | 1 + .../routers/workspaces/utils/github/cache.ts | 2 +- .../routers/workspaces/utils/github/github.ts | 27 +++++++++++++++---- 3 files changed, 24 insertions(+), 6 deletions(-) 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 6ba10d7dfa8..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 @@ -237,6 +237,7 @@ export const createGitStatusProcedures = () => { input, githubStatus: cachedGitHubStatus, }), + branchName: workspace.type === "branch" ? workspace.branch : 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 d884ed95677..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, @@ -114,10 +125,13 @@ async function refreshGitHubPRStatus( execGitWithShellPath(["rev-parse", revParseTarget], { cwd: worktreePath, }).catch((error) => { - if (!branchOverride && isUnbornHeadError(error)) { + if (isUnbornHeadError(error)) { + return { stdout: "", stderr: "" }; + } + if (branchOverride) { return { stdout: "", stderr: "" }; } - return { stdout: "", stderr: "" }; + throw error; }), execGitWithShellPath(["rev-parse", "--abbrev-ref", upstreamTarget], { cwd: worktreePath, @@ -228,13 +242,16 @@ export async function fetchGitHubPRStatus( 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 []; } From b2cf68c074d1166d10062be99f43f62e5d7b8884 Mon Sep 17 00:00:00 2001 From: Tom Whittle Date: Thu, 9 Apr 2026 16:09:38 +0100 Subject: [PATCH 3/3] test: assert getWorktree is not called for branch workspaces --- .../lib/trpc/routers/workspaces/procedures/git-status.test.ts | 2 ++ 1 file changed, 2 insertions(+) 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 index 851c398788c..3dd93a28882 100644 --- 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 @@ -118,6 +118,7 @@ describe("getGitHubStatus", () => { }; mockFetchGitHubPRStatus.mockClear(); + mockGetWorktree.mockClear(); mockGetWorkspace.mockReturnValue(branchWorkspace); mockGetWorkspacePath.mockReturnValue("/repos/my-project"); mockFetchGitHubPRStatus.mockResolvedValue(fakeGitHubStatus); @@ -132,6 +133,7 @@ describe("getGitHubStatus", () => { "/repos/my-project", "feature/branch-workspace", ); + expect(mockGetWorktree).not.toHaveBeenCalled(); }); test("returns GitHub status for worktree workspaces", async () => {