Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 44 additions & 9 deletions apps/desktop/src/lib/forge/forgeFactory.svelte.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { AzureDevOps } from "$lib/forge/azure/azure";
import { BitBucket } from "$lib/forge/bitbucket/bitbucket";
import { AZURE_DOMAIN, AzureDevOps } from "$lib/forge/azure/azure";
import { BitBucket, BITBUCKET_DOMAIN } from "$lib/forge/bitbucket/bitbucket";
import { DefaultForge } from "$lib/forge/default/default";
import { GitHub } from "$lib/forge/github/github";
import { GitHub, GITHUB_DOMAIN } from "$lib/forge/github/github";
import { GitHubClient } from "$lib/forge/github/githubClient";
import { GitLab } from "$lib/forge/gitlab/gitlab";
import { GitLab, GITLAB_DOMAIN, GITLAB_SUB_DOMAIN } from "$lib/forge/gitlab/gitlab";
import { InjectionToken } from "@gitbutler/core/context";
import { deepCompare } from "@gitbutler/shared/compare";
import type { ForgeProvider } from "$lib/baseBranch/baseBranch";
Expand Down Expand Up @@ -113,16 +113,16 @@ export class DefaultForgeFactory implements Reactive<Forge> {
} = config;
this._githubError = githubError;
if (repo && baseBranch) {
const forgeType = forgeOverride ?? detectedForgeProvider ?? "default";
this._determinedForgeType = forgeType;
this._determinedForgeType = this.determineForgeType(repo, detectedForgeProvider);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep determined forge type aligned with effective forge choice

determinedForgeType is set from determineForgeType(...) without considering forgeOverride, but build() can still switch to an override when detection returns default. In that case state becomes inconsistent (current is GitHub/GitLab while determinedForgeType stays default), which suppresses integration UI paths that gate on determinedForgeType !== "default" (e.g. the forge integration banner).

Useful? React with 👍 / 👎.

this._forge = this.build({
repo,
pushRepo,
baseBranch,
forgeType,
githubAuthenticated,
forgeIsLoading,
gitlabAuthenticated,
detectedForgeProvider,
forgeOverride,
});
Comment on lines 115 to 126
} else {
this._determinedForgeType = "default";
Expand All @@ -134,19 +134,25 @@ export class DefaultForgeFactory implements Reactive<Forge> {
repo,
pushRepo,
baseBranch,
forgeType,
githubAuthenticated,
forgeIsLoading,
gitlabAuthenticated,
detectedForgeProvider,
forgeOverride,
}: {
repo: RepoInfo;
pushRepo?: RepoInfo;
baseBranch: string;
forgeType: ForgeName;
githubAuthenticated?: boolean;
forgeIsLoading?: boolean;
gitlabAuthenticated?: boolean;
detectedForgeProvider: ForgeProvider | undefined;
forgeOverride: ForgeName | undefined;
}): Forge {
let forgeType = this.determineForgeType(repo, detectedForgeProvider);
if (forgeType === "default" && forgeOverride) {
forgeType = forgeOverride;
Comment on lines +153 to +154

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Respect forge override even when provider detection succeeds

Apply forgeOverride before (or instead of) detectedForgeProvider here; with the current if (forgeType === "default" && forgeOverride) guard, any non-default detection result permanently wins and user/project overrides are ignored. This regresses cases where a repo has an explicit forge_override configured (e.g. migration or mis-detected hosts), because the factory will instantiate the detected forge instead of the requested one.

Useful? React with 👍 / 👎.

}
Comment on lines +152 to +155
const forkStr =
pushRepo && pushRepo.hash !== repo.hash ? `${pushRepo.owner}:${pushRepo.name}` : undefined;

Expand Down Expand Up @@ -192,6 +198,35 @@ export class DefaultForgeFactory implements Reactive<Forge> {
return this.default;
}

private determineForgeType(
repo: RepoInfo,
detectedForgeProvider: ForgeProvider | undefined,
): ForgeName {
if (detectedForgeProvider) {
return detectedForgeProvider;
}
const domain = repo.domain;

if (domain.includes(GITHUB_DOMAIN)) {
return "github";
}
if (
domain === GITLAB_DOMAIN ||
domain.startsWith(GITLAB_SUB_DOMAIN + ".") ||
domain.startsWith("xy" + GITLAB_SUB_DOMAIN + ".") // Temporary workaround until we have foerge overrides implemented
) {
Comment on lines +213 to +217
return "gitlab";
}
if (domain.includes(BITBUCKET_DOMAIN)) {
return "bitbucket";
}
if (domain.includes(AZURE_DOMAIN)) {
return "azure";
Comment on lines +208 to +224
}

return "default";
}

invalidate(tags: TagDescription<ReduxTag>[]) {
const action = this.current.invalidate(tags);
const { dispatch } = this.params;
Expand Down
81 changes: 37 additions & 44 deletions apps/desktop/src/lib/forge/forgeFactory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,13 @@ describe.concurrent("DefaultforgeFactory", () => {
owner: "test-owner",
},
baseBranch: "some-base",
forgeType: "github",
detectedForgeProvider: undefined,
forgeOverride: undefined,
}),
).instanceOf(GitHub);
});

test("Create GitLab service", async () => {
test("Create self hosted Gitlab service", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
Expand All @@ -70,17 +71,18 @@ describe.concurrent("DefaultforgeFactory", () => {
expect(
factory.build({
repo: {
domain: "gitlab.com",
domain: "gitlab.domain.com",
name: "test-repo",
owner: "test-owner",
},
baseBranch: "some-base",
forgeType: "gitlab",
detectedForgeProvider: undefined,
forgeOverride: undefined,
}),
).instanceOf(GitLab);
});

test("setConfig uses detectedForgeProvider when present", async () => {
test("Create Gitlab service", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
Expand All @@ -90,17 +92,21 @@ describe.concurrent("DefaultforgeFactory", () => {
posthog,
dispatch,
});
factory.setConfig({
repo: { domain: "github.example.net", name: "test-repo", owner: "test-owner" },
baseBranch: "main",
detectedForgeProvider: "github",
forgeOverride: undefined,
});
expect(factory.current).instanceOf(GitHub);
expect(factory.determinedForgeType).toBe("github");
expect(
factory.build({
repo: {
domain: "gitlab.com",
name: "test-repo",
owner: "test-owner",
},
baseBranch: "some-base",
detectedForgeProvider: undefined,
forgeOverride: undefined,
}),
).instanceOf(GitLab);
});

test("setConfig falls back to detectedForgeProvider when forgeOverride is absent", async () => {
test("Respects detectedForgeProvider: GitHub", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
Expand All @@ -110,52 +116,39 @@ describe.concurrent("DefaultforgeFactory", () => {
posthog,
dispatch,
});
factory.setConfig({
repo: { domain: "github.example.net", name: "test-repo", owner: "test-owner" },
const result = factory.build({
repo: {
domain: "gitlab.com",
name: "test-repo",
owner: "test-owner",
},
baseBranch: "main",
detectedForgeProvider: "github",
forgeOverride: undefined,
});
expect(factory.current).instanceOf(GitHub);
expect(factory.determinedForgeType).toBe("github");
expect(result).instanceOf(GitHub);
});

test("setConfig resolves to default when both detectedForgeProvider and forgeOverride are absent", async () => {
test("Respects detectedForgeProvider: GitLab", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
backendApi,
gitLabClient,
gitLabApi,
posthog,
dispatch,
});
factory.setConfig({
repo: { domain: "custom.example.com", name: "test-repo", owner: "test-owner" },
baseBranch: "main",
detectedForgeProvider: undefined,
forgeOverride: undefined,
});
expect(factory.determinedForgeType).toBe("default");
});

test("forgeOverride takes precedence over detectedForgeProvider", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
backendApi,
gitLabClient,
gitLabApi,
posthog,
dispatch,
});
factory.setConfig({
repo: { domain: "github.com", name: "test-repo", owner: "test-owner" },
const result = factory.build({
repo: {
domain: "github.com",
name: "test-repo",
owner: "test-owner",
},
baseBranch: "main",
detectedForgeProvider: "github",
forgeOverride: "gitlab",
detectedForgeProvider: "gitlab",
forgeOverride: undefined,
});
expect(factory.current).instanceOf(GitLab);
expect(factory.determinedForgeType).toBe("gitlab");
expect(result).instanceOf(GitLab);
});
});
22 changes: 1 addition & 21 deletions crates/but-forge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,7 @@ pub fn get_all_forge_accounts() -> anyhow::Result<Vec<ForgeUser>> {
#[cfg(test)]
mod tests {
use super::{
ForgeName, ForgeUser, derive_forge_repo_info, match_host_to_accounts_custom_host,
normalize_host_for_comparison,
ForgeName, ForgeUser, match_host_to_accounts_custom_host, normalize_host_for_comparison,
};

#[test]
Expand Down Expand Up @@ -281,23 +280,4 @@ mod tests {
"repository.com"
);
}

#[test]
fn derive_forge_detects_github_enterprise_subdomain_ssh() {
let info = derive_forge_repo_info("git@github.ourinternaldomain.net:our-org/my-repo.git")
.expect("should detect GHES from github.* subdomain");
assert_eq!(info.forge, ForgeName::GitHub);
assert_eq!(info.owner, "our-org");
assert_eq!(info.repo, "my-repo");
}

#[test]
fn derive_forge_detects_github_enterprise_subdomain_https() {
let info =
derive_forge_repo_info("https://github.ourinternaldomain.net/our-org/my-repo.git")
.expect("should detect GHES from github.* subdomain");
assert_eq!(info.forge, ForgeName::GitHub);
assert_eq!(info.owner, "our-org");
assert_eq!(info.repo, "my-repo");
}
}
Loading