From 4574abc294eebd0f9be27c256bc93150261ad764 Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Mon, 27 Apr 2026 01:20:40 +0800 Subject: [PATCH] refactor: address SonarCloud issues and reduce code duplication (#20) * ci: use shared obsidian plugin workflow and update obsidian version * Add quality gate status badge to README Added a quality gate status badge to the README. * refactor: address SonarCloud issues and reduce code duplication --------- Co-authored-by: ClaudiaFang --- .github/workflows/ci.yml | 2 +- src/logic/sync-manager.ts | 110 +++++++++++++++---------------- src/main.ts | 79 +++++++++------------- src/ui/SyncStatusView.ts | 31 +-------- tests/logic/sync-manager.test.ts | 24 +++++++ 5 files changed, 108 insertions(+), 138 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2e4bc46..ae52bf1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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 }} diff --git a/src/logic/sync-manager.ts b/src/logic/sync-manager.ts index a913f21..fc383ef 100644 --- a/src/logic/sync-manager.ts +++ b/src/logic/sync-manager.ts @@ -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 { + this.settings.syncMetadata[path] = { + lastSyncedSha: sha, + lastSyncedAt: Date.now(), + lastKnownPath: path + }; + await this.saveSettings(); + } + updateGitService(gitService: GitServiceInterface): void { this.gitService = gitService; } @@ -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; @@ -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)}`); } } @@ -88,8 +100,6 @@ export class SyncManager { } private async handleRename(file: TFile, oldPath: string, content: string): Promise { - const serviceName = this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub'; - try { // Push the file to the new location await this.gitService.pushFile( @@ -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, @@ -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); } @@ -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); @@ -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 { + 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() { @@ -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); } } diff --git a/src/main.ts b/src/main.ts index 6056fa6..5a207f1 100644 --- a/src/main.ts +++ b/src/main.ts @@ -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); @@ -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) { @@ -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) { @@ -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); }); }); @@ -115,6 +113,10 @@ export default class GitLabFilesPush extends Plugin { ); } + private get serviceName(): string { + return this.settings.serviceType === 'gitlab' ? 'GitLab' : 'GitHub'; + } + async activateSyncStatusView(): Promise { const { workspace } = this.app; @@ -137,74 +139,53 @@ export default class GitLabFilesPush extends Plugin { } async pushAllFiles(): Promise { - 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 { + await this.runAllFiles('pull'); + } + + private async runAllFiles(op: 'push' | 'pull'): Promise { 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 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)}`); } } diff --git a/src/ui/SyncStatusView.ts b/src/ui/SyncStatusView.ts index 17df172..843c969 100644 --- a/src/ui/SyncStatusView.ts +++ b/src/ui/SyncStatusView.ts @@ -314,34 +314,16 @@ export class SyncStatusView extends ItemView { private async runSingleFile(fileStatus: FileStatus, op: 'push' | 'pull'): Promise { try { - const originalStatus = fileStatus.status; fileStatus.status = 'checking'; this.renderView(); if (op === 'push') { await this.plugin.sync.pushFile(fileStatus.file || fileStatus.path); - await new Promise(r => setTimeout(r, 500)); - } else if (originalStatus === 'remote-only' || !fileStatus.file) { - // pull remote-only - const remote = await this.plugin.gitService.getFile(fileStatus.path, this.plugin.settings.branch); - if (remote.content) { - await this.ensureParentDirs(fileStatus.path); - await this.app.vault.adapter.write(fileStatus.path, remote.content); - this.plugin.settings.syncMetadata[fileStatus.path] = { - lastSyncedSha: remote.sha, - lastSyncedAt: Date.now(), - lastKnownPath: fileStatus.path - }; - await this.plugin.saveSettings(); - } - await new Promise(r => setTimeout(r, 1000)); - await this.refreshAllStatuses(); - return; } else { await this.plugin.sync.pullFile(fileStatus.file || fileStatus.path); - await new Promise(r => setTimeout(r, 500)); } + await new Promise(r => setTimeout(r, 500)); await this.refreshFileStatus(fileStatus.file || fileStatus.path); this.renderView(); } catch (e) { @@ -681,17 +663,6 @@ export class SyncStatusView extends ItemView { return diff.join('\n'); } - private async ensureParentDirs(filePath: string): Promise { - 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 */ } - } - } - } - async pushAllModified(): Promise { await this.runBatchOperation('modified', 'push'); } diff --git a/tests/logic/sync-manager.test.ts b/tests/logic/sync-manager.test.ts index 91cd0d0..4d897a9 100644 --- a/tests/logic/sync-manager.test.ts +++ b/tests/logic/sync-manager.test.ts @@ -28,6 +28,13 @@ const mockApp = { read: vi.fn(), modify: vi.fn(), getFileByPath: vi.fn(), + getAbstractFileByPath: vi.fn(), + createFolder: vi.fn(), + adapter: { + exists: vi.fn(), + read: vi.fn(), + write: vi.fn(), + } } } as unknown as App; @@ -313,6 +320,23 @@ describe('SyncManager', () => { expect(mockApp.vault.modify).not.toHaveBeenCalled(); }); + it('should pull a new file that does not exist locally', async () => { + const path = 'new-remote-file.md'; + vi.mocked(mockGitLab.getFile).mockResolvedValue({ content: 'remote content', sha: 'new-sha' }); + vi.spyOn(mockApp.vault, 'getFileByPath').mockReturnValue(null); + + const writeSpy = vi.spyOn(mockApp.vault.adapter, 'write').mockResolvedValue(undefined); + vi.spyOn(mockApp.vault.adapter, 'exists').mockResolvedValue(false); + + // Mock ensureParentDirs by mocking getAbstractFileByPath to return folder for parent + vi.spyOn(mockApp.vault, 'getAbstractFileByPath').mockReturnValue(new TFile()); + + await manager.pullFile(path); + + expect(writeSpy).toHaveBeenCalledWith(path, 'remote content'); + expect(mockSettings.syncMetadata[path]?.lastSyncedSha).toBe('new-sha'); + }); + it('should handle pull errors gracefully', async () => { const mockFile = Object.assign(new TFile(), { path: 'fail.md', name: 'fail.md' }); vi.mocked(mockGitLab.getFile).mockRejectedValue(new Error('Network error'));