diff --git a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts index 8da5f2ba06..848626b9d3 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/git-operations.ts @@ -8,6 +8,11 @@ import { } from "../workspaces/utils/shell-env"; import { isUpstreamMissingError } from "./git-utils"; import { assertRegisteredWorktree } from "./security"; +import { + buildPullRequestCompareUrl, + normalizeGitHubRepoUrl, + parseUpstreamRef, +} from "./utils/pull-request-url"; import { clearStatusCacheForWorktree } from "./utils/status-cache"; export { isUpstreamMissingError }; @@ -147,11 +152,6 @@ async function getTrackingBranchStatus( } } -function extractPRUrl(text: string): string | null { - const match = text.match(/https:\/\/github\.com\/\S+\/pull\/\d+/); - return match?.[0] ?? null; -} - async function findExistingOpenPRUrl( worktreePath: string, ): Promise { @@ -242,22 +242,92 @@ async function findOpenPRByHeadCommit( } } -async function openPRInBrowser( - worktreePath: string, - prUrl: string, -): Promise { +const ghRepoMetadataSchema = z.object({ + url: z.string().url(), + isFork: z.boolean(), + parent: z + .object({ + url: z.string().url(), + }) + .nullable(), + defaultBranchRef: z.object({ + name: z.string().min(1), + }), +}); + +async function getMergeBaseBranch( + git: ReturnType, + branch: string, +): Promise { try { - await execWithShellEnv("gh", ["pr", "view", prUrl, "--web"], { - cwd: worktreePath, + const configuredBaseBranch = await git.raw([ + "config", + "--get", + `branch.${branch}.gh-merge-base`, + ]); + return configuredBaseBranch.trim() || null; + } catch { + return null; + } +} + +async function buildNewPullRequestUrl( + worktreePath: string, + git: ReturnType, + branch: string, +): Promise { + const { stdout } = await execWithShellEnv( + "gh", + ["repo", "view", "--json", "url,isFork,parent,defaultBranchRef"], + { cwd: worktreePath }, + ); + const repoMetadata = ghRepoMetadataSchema.parse(JSON.parse(stdout)); + const currentRepoUrl = normalizeGitHubRepoUrl(repoMetadata.url); + const baseRepoUrl = normalizeGitHubRepoUrl( + repoMetadata.isFork && repoMetadata.parent?.url + ? repoMetadata.parent.url + : repoMetadata.url, + ); + + if (!currentRepoUrl || !baseRepoUrl) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "GitHub is not available for this workspace.", }); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - console.warn( - `[git/openPRInBrowser] Failed to open PR URL ${prUrl}:`, - message, - ); - // Opening in browser is best-effort; return URL either way. } + + const configuredBaseBranch = await getMergeBaseBranch(git, branch); + const baseBranch = configuredBaseBranch ?? repoMetadata.defaultBranchRef.name; + let headRepoOwner = currentRepoUrl.split("/").at(-2) ?? ""; + let headBranch = branch; + + try { + const upstreamRef = ( + await git.raw(["rev-parse", "--abbrev-ref", "@{upstream}"]) + ).trim(); + const parsedUpstreamRef = parseUpstreamRef(upstreamRef); + + if (parsedUpstreamRef) { + headBranch = parsedUpstreamRef.branchName; + const upstreamRemoteUrl = await git.raw([ + "remote", + "get-url", + parsedUpstreamRef.remoteName, + ]); + headRepoOwner = + normalizeGitHubRepoUrl(upstreamRemoteUrl)?.split("/").at(-2) ?? + headRepoOwner; + } + } catch { + // Fall back to the current repository owner and local branch name. + } + + return buildPullRequestCompareUrl({ + baseRepoUrl, + baseBranch, + headRepoOwner, + headBranch, + }); } async function getGitWithShellPath(worktreePath: string) { @@ -449,20 +519,21 @@ export const createGitOperationsRouter = () => { const existingPRUrl = await findExistingOpenPRUrl(input.worktreePath); if (existingPRUrl) { - await openPRInBrowser(input.worktreePath, existingPRUrl); await fetchCurrentBranch(git); clearStatusCacheForWorktree(input.worktreePath); return { success: true, url: existingPRUrl }; } - let stdout = ""; try { - const result = await execWithShellEnv( - "gh", - ["pr", "create", "--web", "--fill", "--head", branch], - { cwd: input.worktreePath }, + const url = await buildNewPullRequestUrl( + input.worktreePath, + git, + branch, ); - stdout = result.stdout; + await fetchCurrentBranch(git); + clearStatusCacheForWorktree(input.worktreePath); + + return { success: true, url }; } catch (error) { // If creation reports branch/tracking mismatch but an open PR exists, // recover by opening that existing PR instead of failing. @@ -470,20 +541,12 @@ export const createGitOperationsRouter = () => { input.worktreePath, ); if (recoveredPRUrl) { - await openPRInBrowser(input.worktreePath, recoveredPRUrl); await fetchCurrentBranch(git); clearStatusCacheForWorktree(input.worktreePath); return { success: true, url: recoveredPRUrl }; } throw error; } - - const url = - (extractPRUrl(stdout) ?? stdout.trim()) || "https://github.com"; - await fetchCurrentBranch(git); - clearStatusCacheForWorktree(input.worktreePath); - - return { success: true, url }; }, ), diff --git a/apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-url.test.ts b/apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-url.test.ts new file mode 100644 index 0000000000..eb669ea323 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-url.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, test } from "bun:test"; +import { + buildPullRequestCompareUrl, + normalizeGitHubRepoUrl, + parseUpstreamRef, +} from "./pull-request-url"; + +describe("pull-request-url", () => { + test("normalizes GitHub remote URLs", () => { + expect( + normalizeGitHubRepoUrl("https://github.com/superset-sh/superset.git"), + ).toBe("https://github.com/superset-sh/superset"); + expect(normalizeGitHubRepoUrl("git@github.com:Kitenite/superset.git")).toBe( + "https://github.com/Kitenite/superset", + ); + expect( + normalizeGitHubRepoUrl("ssh://git@github.com/Kitenite/superset.git"), + ).toBe("https://github.com/Kitenite/superset"); + }); + + test("parses upstream refs with slashes in branch names", () => { + expect(parseUpstreamRef("kitenite/kitenite/halved-position")).toEqual({ + remoteName: "kitenite", + branchName: "kitenite/halved-position", + }); + }); + + test("builds compare URLs for fork branches", () => { + expect( + buildPullRequestCompareUrl({ + baseRepoUrl: "https://github.com/superset-sh/superset.git", + baseBranch: "main", + headRepoOwner: "Kitenite", + headBranch: "kitenite/halved-position", + }), + ).toBe( + "https://github.com/superset-sh/superset/compare/main...Kitenite:kitenite/halved-position?expand=1", + ); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-url.ts b/apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-url.ts new file mode 100644 index 0000000000..19de788ea6 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-url.ts @@ -0,0 +1,55 @@ +export interface ParsedUpstreamRef { + remoteName: string; + branchName: string; +} + +export interface PullRequestCompareUrlInput { + baseRepoUrl: string; + baseBranch: string; + headRepoOwner: string; + headBranch: string; +} + +export function normalizeGitHubRepoUrl(remoteUrl: string): string | null { + const trimmedRemoteUrl = remoteUrl.trim(); + const match = [ + /^git@github\.com:(?[^/]+)\/(?[^/]+?)(?:\.git)?$/, + /^ssh:\/\/git@github\.com\/(?[^/]+)\/(?[^/]+?)(?:\.git)?$/, + /^https:\/\/github\.com\/(?[^/]+)\/(?[^/]+?)(?:\.git)?\/?$/, + ] + .map((pattern) => pattern.exec(trimmedRemoteUrl)) + .find((result) => result?.groups?.owner && result.groups.repo); + + if (!match?.groups?.owner || !match.groups.repo) { + return null; + } + + return `https://github.com/${match.groups.owner}/${match.groups.repo}`; +} + +export function parseUpstreamRef( + upstreamRef: string, +): ParsedUpstreamRef | null { + const separatorIndex = upstreamRef.indexOf("/"); + if (separatorIndex <= 0 || separatorIndex === upstreamRef.length - 1) { + return null; + } + + return { + remoteName: upstreamRef.slice(0, separatorIndex), + branchName: upstreamRef.slice(separatorIndex + 1), + }; +} + +export function buildPullRequestCompareUrl({ + baseRepoUrl, + baseBranch, + headRepoOwner, + headBranch, +}: PullRequestCompareUrlInput): string { + const normalizedBaseRepoUrl = + normalizeGitHubRepoUrl(baseRepoUrl) ?? + baseRepoUrl.replace(/\.git$/, "").replace(/\/$/, ""); + + return `${normalizedBaseRepoUrl}/compare/${baseBranch}...${headRepoOwner}:${headBranch}?expand=1`; +} diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx index b69c919e60..a546efbf55 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx @@ -23,7 +23,7 @@ import { ChangesHeader } from "./components/ChangesHeader"; import { CommitInput } from "./components/CommitInput"; import { CommitItem } from "./components/CommitItem"; import { FileList } from "./components/FileList"; -import { getPRActionState } from "./utils"; +import { getPRActionState, shouldAutoCreatePRAfterPublish } from "./utils"; interface ChangesViewProps { onFileOpen?: ( @@ -364,8 +364,10 @@ export function ChangesView({ onFileOpen, isExpandedView }: ChangesViewProps) { pullCount: status.pullCount, isDefaultBranch, }); - const shouldAutoCreatePRAfterPublish = - hasGitHubRepo && !isDefaultBranch && !hasExistingPR; + const shouldAutoCreatePR = shouldAutoCreatePRAfterPublish({ + hasExistingPR, + isDefaultBranch, + }); return (
@@ -398,7 +400,7 @@ export function ChangesView({ onFileOpen, isExpandedView }: ChangesViewProps) { hasUpstream={status.hasUpstream} hasExistingPR={hasExistingPR} canCreatePR={prActionState.canCreatePR} - shouldAutoCreatePRAfterPublish={shouldAutoCreatePRAfterPublish} + shouldAutoCreatePRAfterPublish={shouldAutoCreatePR} prUrl={prUrl} onRefresh={handleRefresh} /> diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/auto-create-pr-after-publish.test.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/auto-create-pr-after-publish.test.ts new file mode 100644 index 0000000000..450d10b9ce --- /dev/null +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/auto-create-pr-after-publish.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, test } from "bun:test"; +import { shouldAutoCreatePRAfterPublish } from "./auto-create-pr-after-publish"; + +describe("shouldAutoCreatePRAfterPublish", () => { + test("auto-creates after publishing on a non-default branch without an existing PR", () => { + expect( + shouldAutoCreatePRAfterPublish({ + hasExistingPR: false, + isDefaultBranch: false, + }), + ).toBe(true); + }); + + test("does not auto-create on the default branch", () => { + expect( + shouldAutoCreatePRAfterPublish({ + hasExistingPR: false, + isDefaultBranch: true, + }), + ).toBe(false); + }); + + test("does not auto-create when a PR already exists", () => { + expect( + shouldAutoCreatePRAfterPublish({ + hasExistingPR: true, + isDefaultBranch: false, + }), + ).toBe(false); + }); +}); diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/auto-create-pr-after-publish.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/auto-create-pr-after-publish.ts new file mode 100644 index 0000000000..4703b457ab --- /dev/null +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/auto-create-pr-after-publish.ts @@ -0,0 +1,11 @@ +interface AutoCreatePRAfterPublishInput { + hasExistingPR: boolean; + isDefaultBranch: boolean; +} + +export function shouldAutoCreatePRAfterPublish({ + hasExistingPR, + isDefaultBranch, +}: AutoCreatePRAfterPublishInput): boolean { + return !hasExistingPR && !isDefaultBranch; +} diff --git a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/index.ts b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/index.ts index 8dc9b572a8..f9d69774be 100644 --- a/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/index.ts +++ b/apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/utils/index.ts @@ -1,3 +1,4 @@ +export { shouldAutoCreatePRAfterPublish } from "./auto-create-pr-after-publish"; export { formatRelativeDate } from "./date"; export { getPRActionState } from "./pr-action-state"; export { getStatusColor, getStatusIndicator } from "./status"; diff --git a/apps/desktop/src/renderer/screens/main/hooks/useCreateOrOpenPR/useCreateOrOpenPR.ts b/apps/desktop/src/renderer/screens/main/hooks/useCreateOrOpenPR/useCreateOrOpenPR.ts index 4b8ad961c2..b06fb0450d 100644 --- a/apps/desktop/src/renderer/screens/main/hooks/useCreateOrOpenPR/useCreateOrOpenPR.ts +++ b/apps/desktop/src/renderer/screens/main/hooks/useCreateOrOpenPR/useCreateOrOpenPR.ts @@ -24,7 +24,8 @@ export function useCreateOrOpenPR({ void (async () => { try { - await mutateAsync({ worktreePath }); + const result = await mutateAsync({ worktreePath }); + window.open(result.url, "_blank", "noopener,noreferrer"); toast.success("Opening GitHub..."); onSuccess?.(); return; @@ -45,7 +46,11 @@ export function useCreateOrOpenPR({ } try { - await mutateAsync({ worktreePath, allowOutOfDate: true }); + const result = await mutateAsync({ + worktreePath, + allowOutOfDate: true, + }); + window.open(result.url, "_blank", "noopener,noreferrer"); toast.success("Opening GitHub..."); onSuccess?.(); } catch (retryError) {