Skip to content

refactor: consolidate duplicated methods into BaseGitService #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ClaudiaFang merged 3 commits into from
Apr 26, 2026
Merged

refactor: consolidate duplicated methods into BaseGitService #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59aa5f3
refactor: consolidate duplicated methods into BaseGitService
claude Apr 26, 2026
792313b
chore: update package-lock.json metadata
claude Apr 26, 2026
442902b
test: add coverage tests for uncovered service and logic paths
claude Apr 26, 2026
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
40 changes: 5 additions & 35 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 34 additions & 1 deletion src/services/git-service-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,43 @@
return cleanRoot + cleanPath;
}

protected encodeContent(content: string): string {
const bytes = new TextEncoder().encode(content);
let binary = '';
for (let i = 0; i < bytes.byteLength; i++) {
const byte = bytes[i];
if (byte !== undefined) {
binary += String.fromCodePoint(byte);
}
}
return btoa(binary);
}
Comment on lines +91 to +101

@gemini-code-assist gemini-code-assist Bot Apr 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation of encodeContent uses a loop with string concatenation for every byte. This is inefficient for large files as it leads to repeated string allocations and memory pressure. A more performant approach is to convert the Uint8Array to a binary string using Array.from with a mapping function before joining, which is better optimized in modern JavaScript engines.

    protected encodeContent(content: string): string {
        const bytes = new TextEncoder().encode(content);
        const binary = Array.from(bytes, b => String.fromCharCode(b)).join('');
        return btoa(binary);
    }


protected decodeContent(base64: string): string {
const binary = atob(base64.replace(/\s/g, ''));
const bytes = new Uint8Array(binary.length);
for (let i = 0; i < binary.length; i++) {
const cp = binary.codePointAt(i);
bytes[i] = cp !== undefined ? cp : 0;

Check warning on line 108 in src/services/git-service-base.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Unexpected negated condition. 


See more on https://sonarcloud.io/project/issues?id=firstsun-dev_git-files-sync&issues=AZ3HqwSS-HHpeUn7-03U&open=AZ3HqwSS-HHpeUn7-03U&pullRequest=17
}
return new TextDecoder().decode(bytes);
}
Comment on lines +103 to +111

@gemini-code-assist gemini-code-assist Bot Apr 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

In decodeContent, using binary.codePointAt(i) is semantically misleading because atob returns a string where each character represents a single byte (0-255). charCodeAt(i) is more appropriate. Additionally, the loop can be replaced with the more idiomatic Uint8Array.from to improve readability and efficiency.

    protected decodeContent(base64: string): string {
        const binary = atob(base64.replace(/\s/g, ''));
        const bytes = Uint8Array.from(binary, c => c.charCodeAt(0));
        return new TextDecoder().decode(bytes);
    }


protected handleFileNotFound(e: unknown): GitFile {
if (e instanceof Error && e.message.includes('404')) {
return { content: '', sha: '' };
}
throw e;
}

async getRepoGitignores(branch: string): Promise<string[]> {
const allFiles = await this.listFiles(branch);
return allFiles.filter(p => p.endsWith('.gitignore'));
}
Comment on lines +120 to +123

@gemini-code-assist gemini-code-assist Bot Apr 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The implementation of getRepoGitignores has two issues:

  1. Logic Error with rootPath: It calls this.listFiles(branch), which in subclasses (GitHubService, GitLabService) filters results to only include files within the rootPath. This means .gitignore files at the repository root or in parent directories of the rootPath will be missed, even though they correctly apply to files within the vault. This breaks the ignore logic for subdirectory-based vaults.
  2. Filter Precision: p.endsWith('.gitignore') will match files like not-a.gitignore. Standard gitignore files should be named exactly .gitignore or be located in a subdirectory as /.gitignore.

Consider implementing a way to list all files in the repository without the rootPath filter for this specific method.

Suggested change
async getRepoGitignores(branch: string): Promise<string[]> {
const allFiles = await this.listFiles(branch);
return allFiles.filter(p => p.endsWith('.gitignore'));
}
async getRepoGitignores(branch: string): Promise<string[]> {
// Note: subclasses currently filter listFiles by rootPath, which may miss global gitignores
const allFiles = await this.listFiles(branch);
return allFiles.filter(p => p === '.gitignore' || p.endsWith('/.gitignore'));
}


abstract getFile(path: string, branch: string): Promise<GitFile>;
abstract pushFile(path: string, content: string, branch: string, message: string, sha?: string): Promise<string>;
abstract listFiles(branch: string): Promise<string[]>;
abstract deleteFile(path: string, branch: string, message: string): Promise<void>;
abstract testConnection(): Promise<boolean>;
abstract getRepoGitignores(branch: string): Promise<string[]>;
}
31 changes: 1 addition & 30 deletions src/services/github-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,7 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
sha: data.sha
};
} catch (e) {
if (e instanceof Error && e.message.includes('404')) {
return { content: '', sha: '' };
}
throw e;
return this.handleFileNotFound(e);
}
}

Expand Down Expand Up @@ -86,30 +83,4 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
}
}

async getRepoGitignores(branch: string): Promise<string[]> {
const allFiles = await this.listFiles(branch);
return allFiles.filter(p => p.endsWith('.gitignore'));
}

private encodeContent(content: string): string {
const bytes = new TextEncoder().encode(content);
let binary = '';
for (let i = 0; i < bytes.byteLength; i++) {
const byte = bytes[i];
if (byte !== undefined) {
binary += String.fromCodePoint(byte);
}
}
return btoa(binary);
}

private decodeContent(base64: string): string {
const binary = atob(base64.replace(/\s/g, ''));
const bytes = new Uint8Array(binary.length);
for (let i = 0; i < binary.length; i++) {
const cp = binary.codePointAt(i);
bytes[i] = cp !== undefined ? cp : 0;
}
return new TextDecoder().decode(bytes);
}
}
31 changes: 1 addition & 30 deletions src/services/gitlab-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
sha: data.last_commit_id
};
} catch (e) {
if (e instanceof Error && e.message.includes('404')) {
return { content: '', sha: '' };
}
throw e;
return this.handleFileNotFound(e);
}
}

Expand Down Expand Up @@ -90,30 +87,4 @@ export class GitLabService extends BaseGitService implements GitServiceInterface
}
}

async getRepoGitignores(branch: string): Promise<string[]> {
const allFiles = await this.listFiles(branch);
return allFiles.filter(p => p.endsWith('.gitignore'));
}

private encodeContent(content: string): string {
const bytes = new TextEncoder().encode(content);
let binary = '';
for (let i = 0; i < bytes.byteLength; i++) {
const byte = bytes[i];
if (byte !== undefined) {
binary += String.fromCodePoint(byte);
}
}
return btoa(binary);
}

private decodeContent(base64: string): string {
const binary = atob(base64.replace(/\s/g, ''));
const bytes = new Uint8Array(binary.length);
for (let i = 0; i < binary.length; i++) {
const cp = binary.codePointAt(i);
bytes[i] = cp !== undefined ? cp : 0;
}
return new TextDecoder().decode(bytes);
}
}
28 changes: 28 additions & 0 deletions tests/logic/gitignore-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,34 @@ describe('GitignoreManager', () => {
expect(manager.isIgnored('secret.txt')).toBe(true);
});

it('should fall back to [".gitignore"] when getRepoGitignores throws', async () => {
vi.mocked(mockGitService.getRepoGitignores).mockRejectedValue(new Error('Network error'));
const adapter = mockApp.vault.adapter as Mocked<DataAdapter>;
vi.mocked(adapter.exists).mockResolvedValue(true);
vi.mocked(adapter.read).mockResolvedValue('node_modules/');

const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
await manager.loadGitignores();

expect(consoleSpy).toHaveBeenCalled();
expect(manager.isIgnored('node_modules/test.js')).toBe(true);
});

it('should fall back to remote when local .gitignore read throws', async () => {
vi.mocked(mockGitService.getRepoGitignores).mockResolvedValue(['.gitignore']);
const adapter = mockApp.vault.adapter as Mocked<DataAdapter>;
vi.mocked(adapter.exists).mockResolvedValue(true);
vi.mocked(adapter.read).mockRejectedValue(new Error('Permission denied'));
vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'secret.txt', sha: 'sha' });

const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
await manager.loadGitignores();

expect(consoleSpy).toHaveBeenCalled();
expect(mockGitService.getFile).toHaveBeenCalledWith('/.gitignore', branch);
expect(manager.isIgnored('secret.txt')).toBe(true);
});

it('should handle subdirectory gitignores correctly', async () => {
// Repo structure:
// .gitignore
Expand Down
46 changes: 45 additions & 1 deletion tests/logic/sync-manager-batch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ describe('SyncManager Batch Operations', () => {

it('should handle missing remote files during batch pull', async () => {
const files = ['exists.md', 'missing.md'];

vi.mocked(mockGitService.getFile)
.mockResolvedValueOnce({ content: 'content', sha: 'sha' })
.mockResolvedValueOnce({ content: '', sha: '' });
Expand All @@ -130,4 +130,48 @@ describe('SyncManager Batch Operations', () => {
expect(results.errors[0]!.error).toContain('File not found in remote');
});
});

describe('onProgress callback', () => {
it('should call onProgress for each file processed', async () => {
const files = ['file1.md', 'file2.md', 'file3.md'];
const adapter = mockApp.vault.adapter as Mocked<DataAdapter>;

vi.mocked(adapter.exists).mockResolvedValue(true);
vi.mocked(adapter.read).mockResolvedValue('content');
vi.mocked(mockGitService.getFile).mockResolvedValue({ content: '', sha: 'sha' });
vi.mocked(mockGitService.pushFile).mockResolvedValue('path');

const onProgress = vi.fn();
await manager.pushAllFiles(files, onProgress);

expect(onProgress).toHaveBeenCalledTimes(3);
expect(onProgress).toHaveBeenCalledWith(1, 3, 'file1.md');
expect(onProgress).toHaveBeenCalledWith(2, 3, 'file2.md');
expect(onProgress).toHaveBeenCalledWith(3, 3, 'file3.md');
});
});

describe('batch push with rename detection', () => {
it('should detect and handle rename during batch push', async () => {
const oldPath = 'old.md';
const newPath = 'new.md';
const mockFile = Object.assign(new TFile(), { path: newPath, name: 'new.md' });
mockSettings.syncMetadata = {
[oldPath]: { lastSyncedSha: 'sha', lastSyncedAt: 0, lastKnownPath: oldPath }
};

vi.mocked(mockApp.vault.getFileByPath).mockImplementation(p => p === oldPath ? null : mockFile);
vi.mocked(mockApp.vault.read).mockResolvedValue('content');
vi.mocked(mockApp.vault.adapter.exists as ReturnType<typeof vi.fn>).mockResolvedValue(true);
vi.mocked(mockGitService.pushFile).mockResolvedValue(newPath);
vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'content', sha: 'new-sha' });

const results = await manager.pushAllFiles([mockFile]);

expect(results.success).toBe(1);
expect(mockGitService.pushFile).toHaveBeenCalledWith(
newPath, 'content', 'main', `Rename ${oldPath} to ${newPath}`, undefined
);
});
});
});
Loading
Loading