Skip to content

refactor: eliminate duplication and fix regressions #16

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

Closed
ClaudiaFang wants to merge 7 commits into from
Closed

refactor: eliminate duplication and fix regressions #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
06fb8df
ci: optimize workflow to reduce redundancy and refine permissions
Apr 25, 2026
71d3ac0
ci: fix syntax errors in workflow file
Apr 25, 2026
bf9850c
docs: resolve SonarCloud issues and refactor for complexity
Apr 25, 2026
08fd2b5
refactor: extract BaseGitService to reduce duplication and fix lint/t…
Apr 25, 2026
6c0ca4c
fix: resolve TS build errors due to unchecked indexed access
Apr 25, 2026
26be2e8
refactor: address gemini-code-assist bot reviews
Apr 25, 2026
d44495e
refactor: eliminate duplication by moving encoding logic to BaseGitSe…
Apr 25, 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
9 changes: 9 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,15 @@ jobs:
with:
args: >
-Dsonar.qualitygate.wait=true
-Dsonar.scanner.dumpToFile=sonar-project.properties
- name: Upload Sonar logs
if: always()
uses: actions/upload-artifact@v7
with:
name: sonar-scan-logs
path: |
.scannerwork/
sonar-project.properties

artifact:
name: Package Artifact
Expand Down
74 changes: 39 additions & 35 deletions src/logic/gitignore-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import { App } from 'obsidian';
import { GitServiceInterface } from '../services/git-service-interface';

export class GitignoreManager {
private app: App;
private gitService: GitServiceInterface;
private branch: string;
private readonly app: App;
private readonly gitService: GitServiceInterface;
private readonly branch: string;

private rootPath: string;
private readonly rootPath: string;

// Maps directory path (empty string for root) to Ignore instance
private ignoreMap: Map<string, Ignore> = new Map();
private readonly ignoreMap: Map<string, Ignore> = new Map();

constructor(app: App, gitService: GitServiceInterface, branch: string, rootPath: string) {
this.app = app;
Expand Down Expand Up @@ -38,45 +38,49 @@ export class GitignoreManager {
// 2. Fetch and parse each .gitignore
for (const fullGitignorePath of gitignorePaths) {
const dirPath = fullGitignorePath === '.gitignore' ? '' : fullGitignorePath.slice(0, -('.gitignore'.length + 1));

let content: string | undefined;
const content = await this.getGitignoreContent(fullGitignorePath);

// Determine local path relative to vault root
let localPath: string | null = null;
if (!this.rootPath) {
localPath = fullGitignorePath;
} else if (fullGitignorePath === this.rootPath + '/.gitignore' || fullGitignorePath.startsWith(this.rootPath + '/')) {
localPath = fullGitignorePath.substring(this.rootPath.length + 1);
if (content) {
const ig = ignore().add(content);
this.ignoreMap.set(dirPath, ig);
}
}
}

// Try local first if it's within the vault
if (localPath) {
try {
if (await this.app.vault.adapter.exists(localPath)) {
content = await this.app.vault.adapter.read(localPath);
}
} catch (e) {
console.warn(`Failed to read local ${localPath}`, e);
}
}
private async getGitignoreContent(fullGitignorePath: string): Promise<string | undefined> {
let content: string | undefined;

// Fallback to remote (use absolute path starting with / to bypass rootPath)
if (content === undefined) {
try {
const remoteFile = await this.gitService.getFile('/' + fullGitignorePath, this.branch);
if (remoteFile && remoteFile.content) {
content = remoteFile.content;
}
} catch {
// It's okay if some gitignores fail to fetch
// Determine local path relative to vault root
let localPath: string | null = null;
if (!this.rootPath) {
localPath = fullGitignorePath;
} else if (fullGitignorePath === this.rootPath + '/.gitignore' || fullGitignorePath.startsWith(this.rootPath + '/')) {
localPath = fullGitignorePath.substring(this.rootPath.length + 1);
}

// Try local first if it's within the vault
if (localPath) {
try {
if (await this.app.vault.adapter.exists(localPath)) {
content = await this.app.vault.adapter.read(localPath);
}
} catch (e) {
console.warn(`Failed to read local ${localPath}`, e);
}
}

if (content) {
const ig = ignore().add(content);
this.ignoreMap.set(dirPath, ig);
// Fallback to remote (use absolute path starting with / to bypass rootPath)
if (content === undefined) {
try {
const remoteFile = await this.gitService.getFile('/' + fullGitignorePath, this.branch);
if (remoteFile?.content) {
content = remoteFile.content;
}
} catch {
// It's okay if some gitignores fail to fetch
}
}
return content;
}

/**
Expand Down
125 changes: 71 additions & 54 deletions src/logic/sync-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import { GitLabFilesPushSettings } from '../settings';
import { SyncConflictModal } from '../ui/SyncConflictModal';

export class SyncManager {
private app: App;
private readonly app: App;
private gitService: GitServiceInterface;
private settings: GitLabFilesPushSettings;
private readonly settings: GitLabFilesPushSettings;

constructor(app: App, gitService: GitServiceInterface, settings: GitLabFilesPushSettings) {
this.app = app;
Expand All @@ -19,21 +19,14 @@ export class SyncManager {
}

async pushFile(fileOrPath: TFile | string) {
const isString = typeof fileOrPath === 'string';
const path = isString ? fileOrPath : fileOrPath.path;
const name = isString ? path.split('/').pop() || path : fileOrPath.name;
const { path, name, isString } = this.getFileInfo(fileOrPath);

if (isString) {
if (!(await this.app.vault.adapter.exists(path))) {
new Notice(`File ${name} no longer exists in vault.`);
return;
}
} else if (!this.app.vault.getFileByPath(path)) {
if (!await this.checkFileExists(path, isString)) {
new Notice(`File ${name} no longer exists in vault.`);
return;
}

const content = isString ? await this.app.vault.adapter.read(path) : (fileOrPath instanceof TFile ? await this.app.vault.read(fileOrPath) : '');
const content = await this.getFileContent(fileOrPath);
const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';
try {
// Check if this is a renamed file
Expand All @@ -54,10 +47,11 @@ export class SyncManager {
new SyncConflictModal(this.app, name, content, remote.content, (choice) => {
void (async () => {
try {
const fileRep = typeof fileOrPath === 'string' ? { path, name } : fileOrPath;
if (choice === 'local') {
await this.performPush({ path, name }, content, remote.sha);
} else {
await this.performPull(isString ? { path, name } : fileOrPath, remote.content, remote.sha);
await this.performPull(fileRep, remote.content, remote.sha);
}
} catch (e) {
console.error(e);
Expand Down Expand Up @@ -152,16 +146,9 @@ export class SyncManager {
}

async pullFile(fileOrPath: TFile | string) {
const isString = typeof fileOrPath === 'string';
const path = isString ? fileOrPath : fileOrPath.path;
const name = isString ? path.split('/').pop() || path : fileOrPath.name;
const { path, name, isString } = this.getFileInfo(fileOrPath);

if (isString) {
if (!(await this.app.vault.adapter.exists(path))) {
new Notice(`File ${name} no longer exists in vault.`);
return;
}
} else if (!this.app.vault.getFileByPath(path)) {
if (!await this.checkFileExists(path, isString)) {
new Notice(`File ${name} no longer exists in vault.`);
return;
}
Expand All @@ -173,7 +160,7 @@ export class SyncManager {
new Notice(`File ${name} not found on remote.`);
return;
}
const localContent = isString ? await this.app.vault.adapter.read(path) : (fileOrPath instanceof TFile ? await this.app.vault.read(fileOrPath) : '');
const localContent = await this.getFileContent(fileOrPath);
const lastSynced = this.settings.syncMetadata[path];

if (localContent === remote.content) {
Expand All @@ -192,10 +179,11 @@ export class SyncManager {
new SyncConflictModal(this.app, name, localContent, remote.content, (choice) => {
void (async () => {
try {
const fileRep = typeof fileOrPath === 'string' ? { path, name } : fileOrPath;
if (choice === 'local') {
await this.performPush({ path, name }, localContent, remote.sha);
} else {
await this.performPull(isString ? { path, name } : fileOrPath, remote.content, remote.sha);
await this.performPull(fileRep, remote.content, remote.sha);
}
} catch (e) {
console.error(e);
Expand All @@ -206,7 +194,8 @@ export class SyncManager {
return;
}

await this.performPull(isString ? { path, name } : fileOrPath, remote.content, remote.sha);
const fileRep = typeof fileOrPath === 'string' ? { path, name } : fileOrPath;
await this.performPull(fileRep, remote.content, remote.sha);
} catch (e) {
console.error(e);
new Notice(`Failed to pull ${name} from ${serviceName}: ${e instanceof Error ? e.message : String(e)}`);
Expand All @@ -230,7 +219,7 @@ export class SyncManager {
};

await this.saveSettings();
const name = file instanceof TFile ? file.name : file.name;
const name = file.name;
new Notice(`Pulled ${name} from ${serviceName}`);
}

Expand Down Expand Up @@ -262,42 +251,17 @@ export class SyncManager {
const fileOrPath = files[i];
if (!fileOrPath) continue;

const isString = typeof fileOrPath === 'string';
const path = isString ? fileOrPath : fileOrPath.path;
const name = isString ? path.split('/').pop() || path : fileOrPath.name;
const { path, name, isString } = this.getFileInfo(fileOrPath);

if (onProgress) {
onProgress(i + 1, files.length, name);
}

try {
if (op === 'push') {
let content: string;
if (isString) {
if (!(await this.app.vault.adapter.exists(path))) throw new Error('File no longer exists');
content = await this.app.vault.adapter.read(path);
} else {
const existingFile = this.app.vault.getFileByPath(path);
if (!existingFile) throw new Error('File no longer exists');
content = await this.app.vault.read(existingFile);
}

const remote = await this.gitService.getFile(path, this.settings.branch);
await this.gitService.pushFile(path, content, this.settings.branch, `Update ${name} from Obsidian`, remote.sha || undefined);

const newRemote = await this.gitService.getFile(path, this.settings.branch);
this.settings.syncMetadata[path] = { lastSyncedSha: newRemote.sha, lastSyncedAt: Date.now(), lastKnownPath: path };
await this.processSingleBatchPush(fileOrPath, path, name, isString);
} else {
const remote = await this.gitService.getFile(path, this.settings.branch);
if (!remote.sha) throw new Error('File not found in remote');

if (isString) {
await this.app.vault.adapter.write(path, remote.content);
} else if (fileOrPath instanceof TFile) {
await this.app.vault.modify(fileOrPath, remote.content);
}

this.settings.syncMetadata[path] = { lastSyncedSha: remote.sha, lastSyncedAt: Date.now(), lastKnownPath: path };
await this.processSingleBatchPull(fileOrPath, path, name, isString);
}
results.success++;
} catch (e) {
Expand All @@ -313,4 +277,57 @@ export class SyncManager {

return results;
}

private getFileInfo(fileOrPath: TFile | string) {
const isString = typeof fileOrPath === 'string';
const path = isString ? fileOrPath : fileOrPath.path;
const name = isString ? path.split('/').pop() || path : fileOrPath.name;
return { path, name, isString };
}

private async checkFileExists(path: string, isString: boolean): Promise<boolean> {
if (isString) {
return await this.app.vault.adapter.exists(path);
}
return !!this.app.vault.getFileByPath(path);
}

private async getFileContent(fileOrPath: TFile | string): Promise<string> {
if (typeof fileOrPath === 'string') {
return await this.app.vault.adapter.read(fileOrPath);
}
return await this.app.vault.read(fileOrPath);
}

private async processSingleBatchPush(fileOrPath: TFile | string, path: string, name: string, isString: boolean) {
if (!await this.checkFileExists(path, isString)) throw new Error('File no longer exists');
const content = await this.getFileContent(fileOrPath);

// Rename detection
if (!isString && fileOrPath instanceof TFile) {
const renamedFrom = this.detectRename(fileOrPath);
if (renamedFrom) {
await this.handleRename(fileOrPath, renamedFrom, content);
return;
}
}

const remote = await this.gitService.getFile(path, this.settings.branch);
await this.gitService.pushFile(path, content, this.settings.branch, `Update ${name} from Obsidian`, remote.sha || undefined);
const newRemote = await this.gitService.getFile(path, this.settings.branch);
this.settings.syncMetadata[path] = { lastSyncedSha: newRemote.sha, lastSyncedAt: Date.now(), lastKnownPath: path };
Comment on lines +315 to +318

@gemini-code-assist gemini-code-assist Bot Apr 25, 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 a batch push operation, performing two extra getFile calls per file (one before to get the SHA and one after to update metadata) is inefficient and doubles the network overhead.

  1. The existingSha is often already known from the preceding conflict check or local metadata.
  2. The pushFile API response from GitHub (and commit info from GitLab) can be used to update metadata without an additional fetch. Consider updating the pushFile interface to return the new SHA.

}

private async processSingleBatchPull(fileOrPath: TFile | string, path: string, name: string, isString: boolean) {
const remote = await this.gitService.getFile(path, this.settings.branch);
if (!remote.sha) throw new Error('File not found in remote');

if (typeof fileOrPath === 'string') {
await this.app.vault.adapter.write(fileOrPath, remote.content);
} else if (fileOrPath instanceof TFile) {
await this.app.vault.modify(fileOrPath, remote.content);
}

this.settings.syncMetadata[path] = { lastSyncedSha: remote.sha, lastSyncedAt: Date.now(), lastKnownPath: path };
}
}
Loading
Loading