Skip to content

refactor: address SonarCloud issues and reduce code duplication #20

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 5 commits into from
Apr 26, 2026
Merged

refactor: address SonarCloud issues and reduce code duplication #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
567a984
ci: use shared obsidian plugin workflow and update obsidian version
Apr 26, 2026
909336b
Add quality gate status badge to README
ClaudiaFang Apr 26, 2026
b3c91cf
refactor: address SonarCloud issues and reduce code duplication
Apr 26, 2026
803df6c
Merge branch 'master' into ci/use-shared-workflow
ClaudiaFang Apr 26, 2026
38a722b
ci: test conditional sonar check
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
uses: firstsun-dev/general-workflows/.github/workflows/obsidian-plugin-ci.yml@main
with:
plugin-id: "git-file-sync"
skip-sonar: false
skip-sonar: ${{ secrets.SONAR_TOKEN == '' || vars.ENABLE_CI_SONAR != 'true' }}
secrets:
RELEASE_TOKEN: ${{ secrets.RELEASE_TOKEN }}
SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
110 changes: 52 additions & 58 deletions src/logic/sync-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ export class SyncManager {
this.settings = settings;
}

private get serviceName(): string {
return this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';
}

public async updateMetadata(path: string, sha: string): Promise<void> {
this.settings.syncMetadata[path] = {
lastSyncedSha: sha,
lastSyncedAt: Date.now(),
lastKnownPath: path
};
await this.saveSettings();
}

updateGitService(gitService: GitServiceInterface): void {
this.gitService = gitService;
}
Expand All @@ -27,7 +40,6 @@ export class SyncManager {
}

const content = await this.getFileContent(fileOrPath);
const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';
try {
// Check if this is a renamed file
let renamedFrom = null;
Expand Down Expand Up @@ -65,7 +77,7 @@ export class SyncManager {
await this.performPush({ path, name }, content, remote.sha);
} catch (e) {
console.error(e);
new Notice(`Failed to push ${name} to ${serviceName}: ${e instanceof Error ? e.message : String(e)}`);
new Notice(`Failed to push ${name} to ${this.serviceName}: ${e instanceof Error ? e.message : String(e)}`);
}
}

Expand All @@ -88,8 +100,6 @@ export class SyncManager {
}

private async handleRename(file: TFile, oldPath: string, content: string): Promise<void> {
const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';

try {
// Push the file to the new location
await this.gitService.pushFile(
Expand All @@ -106,25 +116,20 @@ export class SyncManager {

// Update metadata
const newRemote = await this.gitService.getFile(file.path, this.settings.branch);
this.settings.syncMetadata[file.path] = {
lastSyncedSha: newRemote.sha,
lastSyncedAt: Date.now(),
lastKnownPath: file.path
};
await this.updateMetadata(file.path, newRemote.sha);

// Remove old metadata
delete this.settings.syncMetadata[oldPath];

await this.saveSettings();
new Notice(`Renamed and pushed ${file.name} to ${serviceName}\nNote: Old file at ${oldPath} may need manual deletion from remote`);
new Notice(`Renamed and pushed ${file.name} to ${this.serviceName}\nNote: Old file at ${oldPath} may need manual deletion from remote`);
} catch (e) {
console.error(e);
new Notice(`Failed to handle rename: ${e instanceof Error ? e.message : String(e)}`);
}
}

private async performPush(file: {path: string, name: string}, content: string, existingSha?: string) {
const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';
private async performPush(file: {path: string, name: string}, content: string, existingSha?: string, silent = false) {
await this.gitService.pushFile(
file.path,
content,
Expand All @@ -135,53 +140,40 @@ export class SyncManager {

// Update metadata
const newRemote = await this.gitService.getFile(file.path, this.settings.branch);
this.settings.syncMetadata[file.path] = {
lastSyncedSha: newRemote.sha,
lastSyncedAt: Date.now(),
lastKnownPath: file.path
};

await this.saveSettings();
new Notice(`Pushed ${file.name} to ${serviceName}`);
await this.updateMetadata(file.path, newRemote.sha);

if (!silent) new Notice(`Pushed ${file.name} to ${this.serviceName}`);
}

async pullFile(fileOrPath: TFile | string) {
const { path, name, isString } = this.getFileInfo(fileOrPath);

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

const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';
try {
const remote = await this.gitService.getFile(path, this.settings.branch);
if (!remote.sha) {
new Notice(`File ${name} not found on remote.`);
return;
}
const localContent = await this.getFileContent(fileOrPath);

const exists = await this.checkFileExists(path, isString);
const localContent = exists ? await this.getFileContent(fileOrPath) : null;
const lastSynced = this.settings.syncMetadata[path];

if (localContent === remote.content) {
if (exists && localContent === remote.content) {
// Still update metadata even if content matches
this.settings.syncMetadata[path] = {
lastSyncedSha: remote.sha,
lastSyncedAt: Date.now()
};
await this.saveSettings();
await this.updateMetadata(path, remote.sha);
new Notice(`${name} is already up to date.`);
return;
}

// Conflict detection for pull
if (remote.sha && lastSynced && remote.sha !== lastSynced.lastSyncedSha) {
new SyncConflictModal(this.app, name, localContent, remote.content, (choice) => {
// Conflict detection for pull (only if local exists)
if (exists && remote.sha && lastSynced && remote.sha !== lastSynced.lastSyncedSha) {
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);
await this.performPush({ path, name }, localContent || '', remote.sha);
} else {
await this.performPull(fileRep, remote.content, remote.sha);
}
Expand All @@ -198,12 +190,12 @@ export class SyncManager {
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)}`);
new Notice(`Failed to pull ${name} from ${this.serviceName}: ${e instanceof Error ? e.message : String(e)}`);
}
}

private async performPull(file: TFile | {path: string, name: string}, remoteContent: string, remoteSha: string) {
const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';
private async performPull(file: TFile | {path: string, name: string}, remoteContent: string, remoteSha: string, silent = false) {
await this.ensureParentDirs(file.path);

if (file instanceof TFile) {
await this.app.vault.modify(file, remoteContent);
Expand All @@ -212,15 +204,24 @@ export class SyncManager {
}

// Update metadata
this.settings.syncMetadata[file.path] = {
lastSyncedSha: remoteSha,
lastSyncedAt: Date.now(),
lastKnownPath: file.path
};
await this.updateMetadata(file.path, remoteSha);

await this.saveSettings();
const name = file.name;
new Notice(`Pulled ${name} from ${serviceName}`);
if (!silent) new Notice(`Pulled ${file.name} from ${this.serviceName}`);
}

private async ensureParentDirs(filePath: string): Promise<void> {
const parts = filePath.split('/');
let cur = '';
for (let i = 0; i < parts.length - 1; i++) {
cur += (i > 0 ? '/' : '') + parts[i];
if (!this.app.vault.getAbstractFileByPath(cur)) {
try {
await this.app.vault.createFolder(cur);
} catch {
// already exists or failed
}
}
}
}

private async saveSettings() {
Expand Down Expand Up @@ -313,21 +314,14 @@ export class SyncManager {
}

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.performPush({ path, name }, content, remote.sha || undefined, true);
}

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 };
const fileRep = typeof fileOrPath === 'string' ? { path, name } : fileOrPath;
await this.performPull(fileRep, remote.content, remote.sha, true);
}
}
79 changes: 30 additions & 49 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ export default class GitLabFilesPush extends Plugin {
this.gitignoreManager = new GitignoreManager(this.app, this.gitService, this.settings.branch, this.settings.rootPath);
this.sync = new SyncManager(this.app, this.gitService, this.settings);

const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';

this.addRibbonIcon('upload-cloud', Platform.isMobile ? `Push` : `Push to ${serviceName}`, async () => {
this.addRibbonIcon('upload-cloud', Platform.isMobile ? `Push` : `Push to ${this.serviceName}`, async () => {
const activeView = this.app.workspace.getActiveViewOfType(MarkdownView);
if (activeView && activeView.file instanceof TFile) {
await this.sync.pushFile(activeView.file);
Expand All @@ -52,7 +50,7 @@ export default class GitLabFilesPush extends Plugin {

this.addCommand({
id: 'push-current-file',
name: `Push current file to ${serviceName}`,
name: `Push current file to ${this.serviceName}`,
callback: async () => {
const activeView = this.app.workspace.getActiveViewOfType(MarkdownView);
if (activeView && activeView.file instanceof TFile) {
Expand All @@ -63,7 +61,7 @@ export default class GitLabFilesPush extends Plugin {

this.addCommand({
id: 'pull-current-file',
name: `Pull current file from ${serviceName}`,
name: `Pull current file from ${this.serviceName}`,
callback: async () => {
const activeView = this.app.workspace.getActiveViewOfType(MarkdownView);
if (activeView && activeView.file instanceof TFile) {
Expand Down Expand Up @@ -92,12 +90,12 @@ export default class GitLabFilesPush extends Plugin {
this.app.workspace.on('file-menu', (menu, file) => {
if (file instanceof TFile) {
menu.addItem((item) => {
item.setTitle(`Push to ${serviceName}`)
item.setTitle(`Push to ${this.serviceName}`)
.setIcon('upload-cloud')
.onClick(async () => { await this.sync.pushFile(file); });
});
menu.addItem((item) => {
item.setTitle(`Pull from ${serviceName}`)
item.setTitle(`Pull from ${this.serviceName}`)
.setIcon('download-cloud')
.onClick(async () => { await this.sync.pullFile(file); });
});
Expand All @@ -115,6 +113,10 @@ export default class GitLabFilesPush extends Plugin {
);
}

private get serviceName(): string {
return this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';
}

async activateSyncStatusView(): Promise<void> {
const { workspace } = this.app;

Expand All @@ -137,74 +139,53 @@ export default class GitLabFilesPush extends Plugin {
}

async pushAllFiles(): Promise<void> {
const allFiles = this.app.vault.getFiles();
let files = this.filterFilesByVaultFolder(allFiles);
const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';

await this.gitService.listFiles(this.settings.branch);
await this.gitignoreManager.loadGitignores();
files = files.filter(f => !this.gitignoreManager.isIgnored(f.path));

if (files.length === 0) {
new Notice('No files to push in the configured vault folder');
return;
}

const confirmed = await this.showConfirmDialog(`Push ${files.length} file(s) to ${serviceName}?`);
if (!confirmed) return;

const progressNotice = new Notice(`Pushing 0/${files.length} files...`, 0);

try {
const results = await this.sync.pushAllFiles(files, (current, total, fileName) => {
progressNotice.setMessage(`Pushing ${current}/${total}: ${fileName}`);
});

progressNotice.hide();

if (results.errors.length > 0) {
console.error('Push errors:', results.errors);
}
} catch (e) {
progressNotice.hide();
console.error(e);
new Notice(`Push failed: ${e instanceof Error ? e.message : String(e)}`);
}
await this.runAllFiles('push');
}

async pullAllFiles(): Promise<void> {
await this.runAllFiles('pull');
}

private async runAllFiles(op: 'push' | 'pull'): Promise<void> {
const allFiles = this.app.vault.getFiles();
let files = this.filterFilesByVaultFolder(allFiles);
const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub';

await this.gitService.listFiles(this.settings.branch);
await this.gitignoreManager.loadGitignores();
Comment on lines 153 to 154

@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 call to this.gitService.listFiles is redundant as its result is not used, leading to an unnecessary network request. It should be removed to improve the efficiency of batch operations.

Suggested change
await this.gitService.listFiles(this.settings.branch);
await this.gitignoreManager.loadGitignores();
await this.gitignoreManager.loadGitignores();

files = files.filter(f => !this.gitignoreManager.isIgnored(f.path));

if (files.length === 0) {
new Notice('No files to pull in the configured vault folder');
new Notice(`No files to ${op} in the configured vault folder`);
return;
}

const confirmed = await this.showConfirmDialog(`Pull ${files.length} file(s) from ${serviceName}? This will overwrite local changes.`);
const msg = op === 'push'
? `Push ${files.length} file(s) to ${this.serviceName}?`
: `Pull ${files.length} file(s) from ${this.serviceName}? This will overwrite local changes.`;

const confirmed = await this.showConfirmDialog(msg);
if (!confirmed) return;

const progressNotice = new Notice(`Pulling 0/${files.length} files...`, 0);
const progressNotice = new Notice(`${op === 'push' ? 'Pushing' : 'Pulling'} 0/${files.length} files...`, 0);

try {
const results = await this.sync.pullAllFiles(files, (current, total, fileName) => {
progressNotice.setMessage(`Pulling ${current}/${total}: ${fileName}`);
});
const results = op === 'push'
? await this.sync.pushAllFiles(files, (current, total, fileName) => {
progressNotice.setMessage(`Pushing ${current}/${total}: ${fileName}`);
})
: await this.sync.pullAllFiles(files, (current, total, fileName) => {
progressNotice.setMessage(`Pulling ${current}/${total}: ${fileName}`);
});

progressNotice.hide();

if (results.errors.length > 0) {
console.error('Pull errors:', results.errors);
console.error(`${op} errors:`, results.errors);
}
} catch (e) {
progressNotice.hide();
console.error(e);
new Notice(`Pull failed: ${e instanceof Error ? e.message : String(e)}`);
new Notice(`${op === 'push' ? 'Push' : 'Pull'} failed: ${e instanceof Error ? e.message : String(e)}`);
}
}

Expand Down
Loading
Loading