refactor: consolidate duplicated methods into BaseGitService - #17
Conversation
Move encodeContent, decodeContent, getRepoGitignores, and handleFileNotFound from GitHubService and GitLabService into BaseGitService to eliminate code duplication flagged by SonarQube. https://claude.ai/code/session_01BA3xCZgi5JX6Wz4xgF1Hwz
Sync package-lock.json with current package.json (name, version, license). https://claude.ai/code/session_01BA3xCZgi5JX6Wz4xgF1Hwz
Cover listFiles, deleteFile, testConnection, getRepoGitignores in both GitHubService and GitLabService. Add BaseGitService tests for non-Error safeRequest throws and rootPath-ends-with-slash branch. Add gitignore-manager error fallback tests and sync-manager batch onProgress/rename tests. Improves statement coverage from 78.6% to 92.6% and function coverage from 69.1% to 94.5%. https://claude.ai/code/session_01BA3xCZgi5JX6Wz4xgF1Hwz
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Git service layer by moving shared logic for content encoding, decoding, and error handling into the BaseGitService class, reducing duplication across GitHub and GitLab implementations. It also introduces new tests for GitignoreManager fallbacks and SyncManager batch operations. Feedback highlights a logic error in getRepoGitignores where rootPath filtering might exclude necessary ignore files, and suggests performance and semantic improvements for content processing methods. Additionally, it is recommended to separate project metadata and license changes from the primary refactoring effort.
| async getRepoGitignores(branch: string): Promise<string[]> { | ||
| const allFiles = await this.listFiles(branch); | ||
| return allFiles.filter(p => p.endsWith('.gitignore')); | ||
| } |
There was a problem hiding this comment.
The implementation of getRepoGitignores has two issues:
- Logic Error with rootPath: It calls
this.listFiles(branch), which in subclasses (GitHubService,GitLabService) filters results to only include files within therootPath. This means.gitignorefiles at the repository root or in parent directories of therootPathwill be missed, even though they correctly apply to files within the vault. This breaks the ignore logic for subdirectory-based vaults. - Filter Precision:
p.endsWith('.gitignore')will match files likenot-a.gitignore. Standard gitignore files should be named exactly.gitignoreor 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.
| 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')); | |
| } |
| "name": "git-file-sync", | ||
| "version": "1.1.0", | ||
| "lockfileVersion": 3, | ||
| "requires": true, | ||
| "packages": { | ||
| "": { | ||
| "name": "git-file-push", | ||
| "version": "1.0.0", | ||
| "license": "0-BSD", | ||
| "name": "git-file-sync", | ||
| "version": "1.1.0", | ||
| "license": "MIT", |
There was a problem hiding this comment.
This pull request includes significant metadata changes in package-lock.json, such as renaming the project from git-file-push to git-file-sync, bumping the version to 1.1.0, and changing the license from 0-BSD to MIT. These changes are unrelated to the stated goal of refactoring duplicated methods into BaseGitService. It is best practice to keep metadata and licensing changes in separate, dedicated commits or pull requests to maintain a clear audit trail.
| 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); | ||
| } |
There was a problem hiding this comment.
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; | ||
| } | ||
| return new TextDecoder().decode(bytes); | ||
| } |
There was a problem hiding this comment.
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);
}
|
🎉 This PR is included in version 1.0.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
2 participants
Move encodeContent, decodeContent, getRepoGitignores, and handleFileNotFound
from GitHubService and GitLabService into BaseGitService to eliminate
code duplication flagged by SonarQube.
https://claude.ai/code/session_01BA3xCZgi5JX6Wz4xgF1Hwz