fix(sync): clearer branch-not-found errors and connection test

- listFilesDetailed rethrows 404 branch-resolution failures with a
  message naming the branch, instead of a bare "Git Service Error (404)"
- testConnection now also checks the configured branch exists and
  reports repoOk/branchOk separately so the settings UI can warn when
  the repo is reachable but the branch is missing
- fixes settings.ts silently reporting "connection successful" even
  when testConnection's result was never checked

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
ClaudiaFang 2026-07-05 04:57:47 +00:00
parent 1e21061aa8
commit 2f6859a2f1
10 changed files with 147 additions and 31 deletions

View file

@ -43,6 +43,15 @@ export interface GitLabTreeItem {
mode?: string;
}
export interface ConnectionTestResult {
/** Whether the repository/project itself was reachable with the given credentials. */
repoOk: boolean;
/** Whether the configured branch was found. Only meaningful when repoOk is true. */
branchOk: boolean;
/** Populated when repoOk is false, describing the repo-level failure. */
error?: string;
}
export abstract class BaseGitService {
protected token: string = '';
protected rootPath: string = '';
@ -197,6 +206,22 @@ export abstract class BaseGitService {
throw e;
}
/**
* Rethrows a branch-resolution failure (e.g. the "resolve branch to a commit"
* request in listFilesDetailed) with a message that names the branch, since a
* bare "404" gives no clue that the configured Branch setting is the problem.
*/
protected branchNotFoundError(e: unknown, branch: string): Error {
if (e instanceof Error && e.message.includes('404')) {
return new Error(
`Branch "${branch}" was not found in the repository. Check the Branch setting, ` +
'or confirm the repository actually has a branch with this name (its default branch ' +
'might be "master" instead of "main", or the repository may have no commits yet).'
);
}
return e instanceof Error ? e : new Error(String(e));
}
async getRepoGitignores(branch: string): Promise<string[]> {
try {
const allFiles = await this.listFiles(branch, false); // Fetch ALL files to find gitignores
@ -215,5 +240,5 @@ export abstract class BaseGitService {
abstract pushFile(path: string, content: string | ArrayBuffer, branch: string, message: string, sha?: string): Promise<{ path: string, sha?: string }>;
abstract listFilesDetailed(branch: string, useFilter?: boolean): Promise<GitTreeEntry[]>;
abstract deleteFile(path: string, branch: string, message: string): Promise<void>;
abstract testConnection(): Promise<boolean>;
abstract testConnection(branch: string): Promise<ConnectionTestResult>;
}

View file

@ -1,3 +1,5 @@
import { ConnectionTestResult } from './git-service-base';
export interface GitFile {
content: string | ArrayBuffer;
sha: string;
@ -17,7 +19,8 @@ export interface GitServiceInterface {
updateConfig(...args: unknown[]): void;
getFile(path: string, branch: string): Promise<GitFile>;
pushFile(path: string, content: string | ArrayBuffer, branch: string, commitMessage: string, existingSha?: string): Promise<{ path: string, sha?: string }>;
testConnection(): Promise<boolean>;
/** Checks the repository is reachable and the given branch exists. */
testConnection(branch: string): Promise<ConnectionTestResult>;
listFiles(branch: string, useFilter?: boolean): Promise<string[]>;
/** Like listFiles but also reports which entries are symbolic links (mode 120000). */
listFilesDetailed(branch: string, useFilter?: boolean): Promise<GitTreeEntry[]>;

View file

@ -1,5 +1,5 @@
import { GitServiceInterface, GitTreeEntry } from './git-service-interface';
import { BaseGitService, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base';
import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base';
import { logger } from '../utils/logger';
export class GiteaService extends BaseGitService implements GitServiceInterface {
@ -59,9 +59,13 @@ export class GiteaService extends BaseGitService implements GitServiceInterface
// Resolve branch name to commit SHA first for compatibility with all Gitea versions,
// since the git/trees endpoint requires a SHA (not a ref name) on older instances.
const branchUrl = `${this.baseUrl}/api/v1/repos/${this.owner}/${this.repo}/branches/${branch}`;
const branchResponse = await this.safeRequest(branchUrl, 'GET');
const branchData = this.parseJson<{ commit: { id: string } }>(branchResponse);
const commitSha = branchData.commit.id;
let commitSha: string;
try {
const branchResponse = await this.safeRequest(branchUrl, 'GET');
commitSha = this.parseJson<{ commit: { id: string } }>(branchResponse).commit.id;
} catch (e) {
throw this.branchNotFoundError(e, branch);
}
const treeUrl = `${this.baseUrl}/api/v1/repos/${this.owner}/${this.repo}/git/trees/${commitSha}?recursive=1`;
const treeResponse = await this.safeRequest(treeUrl, 'GET');
@ -96,13 +100,20 @@ export class GiteaService extends BaseGitService implements GitServiceInterface
await this.safeRequest(url, 'DELETE', body);
}
async testConnection(): Promise<boolean> {
async testConnection(branch: string): Promise<ConnectionTestResult> {
try {
const url = `${this.baseUrl}/api/v1/repos/${this.owner}/${this.repo}`;
await this.safeRequest(url, 'GET');
return true;
} catch (e) {
return { repoOk: false, branchOk: false, error: e instanceof Error ? e.message : String(e) };
}
try {
const branchUrl = `${this.baseUrl}/api/v1/repos/${this.owner}/${this.repo}/branches/${branch}`;
await this.safeRequest(branchUrl, 'GET', undefined, undefined, true);
return { repoOk: true, branchOk: true };
} catch {
return false;
return { repoOk: true, branchOk: false };
}
}
}

View file

@ -1,5 +1,5 @@
import { GitServiceInterface, GitTreeEntry } from './git-service-interface';
import { BaseGitService, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base';
import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base';
import { logger } from '../utils/logger';
export class GitHubService extends BaseGitService implements GitServiceInterface {
@ -97,8 +97,13 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
async listFilesDetailed(branch: string, useFilter = true): Promise<GitTreeEntry[]> {
const url = `https://api.github.com/repos/${this.owner}/${this.repo}/git/trees/${branch}?recursive=1`;
const response = await this.safeRequest(url, 'GET');
const data = this.parseJson<GitHubTreeResponse>(response);
let data: GitHubTreeResponse;
try {
const response = await this.safeRequest(url, 'GET');
data = this.parseJson<GitHubTreeResponse>(response);
} catch (e) {
throw this.branchNotFoundError(e, branch);
}
if (data.truncated) {
logger.warn('GitHub tree result is truncated. Some files might not be shown.');
@ -129,13 +134,20 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
await this.safeRequest(url, 'DELETE', body);
}
async testConnection(): Promise<boolean> {
async testConnection(branch: string): Promise<ConnectionTestResult> {
try {
const url = `https://api.github.com/repos/${this.owner}/${this.repo}`;
await this.safeRequest(url, 'GET');
return true;
} catch (e) {
return { repoOk: false, branchOk: false, error: e instanceof Error ? e.message : String(e) };
}
try {
const branchUrl = `https://api.github.com/repos/${this.owner}/${this.repo}/branches/${branch}`;
await this.safeRequest(branchUrl, 'GET', undefined, undefined, true);
return { repoOk: true, branchOk: true };
} catch {
return false;
return { repoOk: true, branchOk: false };
}
}

View file

@ -1,5 +1,5 @@
import { GitServiceInterface, GitTreeEntry } from './git-service-interface';
import { BaseGitService, GitFile, GitLabFileResponse, GitLabTreeItem, GIT_SYMLINK_MODE } from './git-service-base';
import { BaseGitService, ConnectionTestResult, GitFile, GitLabFileResponse, GitLabTreeItem, GIT_SYMLINK_MODE } from './git-service-base';
export class GitLabService extends BaseGitService implements GitServiceInterface {
private baseUrl: string = 'https://gitlab.com';
@ -63,8 +63,13 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
while (true) {
const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/tree?ref=${branch}&recursive=true&per_page=${perPage}&page=${page}`;
const response = await this.safeRequest(url, 'GET');
const data = this.parseJson<GitLabTreeItem[]>(response);
let data: GitLabTreeItem[];
try {
const response = await this.safeRequest(url, 'GET');
data = this.parseJson<GitLabTreeItem[]>(response);
} catch (e) {
throw this.branchNotFoundError(e, branch);
}
if (!data || data.length === 0) break;
@ -100,14 +105,22 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
await this.safeRequest(url, 'DELETE', body);
}
async testConnection(): Promise<boolean> {
async testConnection(branch: string): Promise<ConnectionTestResult> {
const encodedProjectId = encodeURIComponent(this.projectId);
try {
const encodedProjectId = encodeURIComponent(this.projectId);
const url = `${this.baseUrl}/api/v4/projects/${encodedProjectId}`;
await this.safeRequest(url, 'GET');
return true;
} catch (e) {
return { repoOk: false, branchOk: false, error: e instanceof Error ? e.message : String(e) };
}
try {
const encodedBranch = encodeURIComponent(branch);
const branchUrl = `${this.baseUrl}/api/v4/projects/${encodedProjectId}/repository/branches/${encodedBranch}`;
await this.safeRequest(branchUrl, 'GET', undefined, undefined, true);
return { repoOk: true, branchOk: true };
} catch {
return false;
return { repoOk: true, branchOk: false };
}
}

View file

@ -174,8 +174,18 @@ export class GitLabSyncSettingTab extends PluginSettingTab {
.setButtonText('Test connection')
.onClick(async () => {
try {
await this.plugin.gitService.testConnection();
new Notice(`${getServiceName(this.plugin.settings)} connection successful!`);
const result = await this.plugin.gitService.testConnection(this.plugin.settings.branch);
if (!result.repoOk) {
new Notice(`Connection failed: ${result.error ?? 'could not reach the repository'}`);
} else if (!result.branchOk) {
new Notice(
`Connected, but branch "${this.plugin.settings.branch}" was not found. ` +
'Check the Branch setting, or confirm the repository has a branch with this name.',
8000
);
} else {
new Notice(`${getServiceName(this.plugin.settings)} connection successful!`);
}
} catch (e: unknown) {
const message = e instanceof Error ? e.message : String(e);
new Notice(`Connection failed: ${message}`);

View file

@ -179,6 +179,11 @@ describe('GiteaService', () => {
expect(result).toEqual(['file1.md']);
warnSpy.mockRestore();
});
it('should throw a message naming the branch when the branch is not found', async () => {
mockRequest({ status: 404, json: { message: 'branch does not exist' }, text: 'branch does not exist' });
await expect(service.listFiles('missing-branch')).rejects.toThrow(/Branch "missing-branch" was not found/);
});
});
describe('deleteFile', () => {
@ -216,9 +221,18 @@ describe('GiteaService', () => {
it('should call the correct repo API URL', async () => {
mockRequest({ status: 200, json: {} });
await service.testConnection();
const call = getLastRequestCall();
expect(call.url).toBe(`${baseUrl}/api/v1/repos/${owner}/${repo}`);
await service.testConnection('main');
const calls = vi.mocked(requestUrl).mock.calls;
const firstCall = calls[0]?.[0] as RequestUrlParam;
expect(firstCall.url).toBe(`${baseUrl}/api/v1/repos/${owner}/${repo}`);
});
it('should report branchOk: false when the branch is not found', async () => {
vi.mocked(requestUrl)
.mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse)
.mockResolvedValueOnce({ status: 404, json: { message: 'not found' }, text: 'not found' } as unknown as RequestUrlResponse);
const result = await service.testConnection('missing-branch');
expect(result).toEqual({ repoOk: true, branchOk: false });
});
});

View file

@ -191,6 +191,11 @@ describe('GitHubService', () => {
expect(result).toEqual(['file1.md', 'file2.md']);
warnSpy.mockRestore();
});
it('should throw a message naming the branch when the branch is not found', async () => {
mockRequest({ status: 404, json: { message: 'Not Found' }, text: 'Not Found' });
await expect(service.listFiles('missing-branch')).rejects.toThrow(/Branch "missing-branch" was not found/);
});
});
describe('deleteFile', () => {
@ -211,6 +216,14 @@ describe('GitHubService', () => {
describe('testConnection', () => {
sharedTestConnection(() => service);
it('should report branchOk: false when the branch is not found', async () => {
vi.mocked(requestUrl)
.mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse)
.mockResolvedValueOnce({ status: 404, json: { message: 'Not Found' }, text: 'Not Found' } as unknown as RequestUrlResponse);
const result = await service.testConnection('missing-branch');
expect(result).toEqual({ repoOk: true, branchOk: false });
});
});
describe('getRepoGitignores', () => {

View file

@ -143,6 +143,11 @@ describe('GitLabService', () => {
expect(result).toHaveLength(142);
expect(vi.mocked(requestUrl)).toHaveBeenCalledTimes(2);
});
it('should throw a message naming the branch when the branch is not found', async () => {
mockRequest({ status: 404, json: { message: '404 Branch Not Found' }, text: '404 Branch Not Found' });
await expect(service.listFiles('missing-branch')).rejects.toThrow(/Branch "missing-branch" was not found/);
});
});
describe('deleteFile', () => {
@ -157,6 +162,14 @@ describe('GitLabService', () => {
describe('testConnection', () => {
sharedTestConnection(() => service);
it('should report branchOk: false when the branch is not found', async () => {
vi.mocked(requestUrl)
.mockResolvedValueOnce({ status: 200, json: {} } as unknown as RequestUrlResponse)
.mockResolvedValueOnce({ status: 404, json: { message: 'Branch Not Found' }, text: 'Branch Not Found' } as unknown as RequestUrlResponse);
const result = await service.testConnection('missing-branch');
expect(result).toEqual({ repoOk: true, branchOk: false });
});
});
describe('getRepoGitignores', () => {

View file

@ -14,14 +14,16 @@ export function mockRequest(response: Partial<RequestUrlResponse>): void {
}
export function sharedTestConnection(getService: () => GitServiceInterface): void {
it('should return true on successful connection', async () => {
it('should report repoOk and branchOk on successful connection', async () => {
mockRequest({ status: 200, json: {} });
expect(await getService().testConnection()).toBe(true);
expect(await getService().testConnection('main')).toEqual({ repoOk: true, branchOk: true });
});
it('should return false on failed connection', async () => {
it('should report repoOk: false on failed connection', async () => {
mockRequest({ status: 401, json: { message: 'Unauthorized' }, text: 'Unauthorized' });
expect(await getService().testConnection()).toBe(false);
const result = await getService().testConnection('main');
expect(result.repoOk).toBe(false);
expect(result.branchOk).toBe(false);
});
}