From 1364b94f0a4063c84649c278990ed7116f95196a Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Sun, 5 Jul 2026 05:05:54 +0000 Subject: [PATCH] fix(sync): stop batch push/pull from silently overwriting conflicts Single-file push/pull already detected when both local and remote changed since the last sync and prompted via SyncConflictModal, but "Push all"/"Pull all"/multi-select batch actions skipped that check and force-overwrote. Batch operations now skip conflicting files and report a conflicts count so nothing is silently clobbered. Also correct delete-confirmation copy: it claimed local deletes are recoverable ("moved to trash") unconditionally, but the actual destination depends on the vault's "Deleted files" setting (which can be permanent). Wording now defers to that setting instead of asserting recoverability. --- src/logic/sync-manager.ts | 65 +++++++++++++++++--------- src/ui/SyncStatusView.ts | 17 +++++-- tests/logic/sync-manager-batch.test.ts | 56 ++++++++++++++++++++++ 3 files changed, 112 insertions(+), 26 deletions(-) diff --git a/src/logic/sync-manager.ts b/src/logic/sync-manager.ts index 35fb9f8..dc0b949 100644 --- a/src/logic/sync-manager.ts +++ b/src/logic/sync-manager.ts @@ -6,6 +6,9 @@ import { logger } from '../utils/logger'; import { isBinaryPath, contentsEqual } from '../utils/path'; import { readLocalSymlinkTarget, createLocalSymlink } from '../utils/symlink'; +/** Result of syncing one file within a batch push/pull. */ +type BatchOutcome = 'done' | 'unchanged' | 'conflict'; + export class SyncManager { private readonly app: App; private gitService: GitServiceInterface; @@ -331,11 +334,11 @@ export class SyncManager { new Notice(`${message}: ${detail}`); } - async pushAllFiles(files: (TFile | string)[], onProgress?: (current: number, total: number, fileName: string) => void): Promise<{ success: number; failed: number; errors: Array<{ file: string; error: string }> }> { + async pushAllFiles(files: (TFile | string)[], onProgress?: (current: number, total: number, fileName: string) => void): Promise<{ success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }> { return this.processBatch(files, 'push', onProgress); } - async pullAllFiles(files: (TFile | string)[], onProgress?: (current: number, total: number, fileName: string) => void): Promise<{ success: number; failed: number; errors: Array<{ file: string; error: string }> }> { + async pullAllFiles(files: (TFile | string)[], onProgress?: (current: number, total: number, fileName: string) => void): Promise<{ success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }> { return this.processBatch(files, 'pull', onProgress); } @@ -343,8 +346,8 @@ export class SyncManager { files: (TFile | string)[], op: 'push' | 'pull', onProgress?: (current: number, total: number, fileName: string) => void - ): Promise<{ success: number; failed: number; errors: Array<{ file: string; error: string }> }> { - const results = { success: 0, failed: 0, errors: [] as Array<{ file: string; error: string }> }; + ): Promise<{ success: number; failed: number; conflicts: number; errors: Array<{ file: string; error: string }> }> { + const results = { success: 0, failed: 0, conflicts: 0, errors: [] as Array<{ file: string; error: string }> }; for (let i = 0; i < files.length; i++) { const fileOrPath = files[i]; @@ -354,13 +357,12 @@ export class SyncManager { onProgress?.(i + 1, files.length, name); try { - let performed = false; - if (op === 'push') { - performed = await this.processSingleBatchPush(fileOrPath, path, name, isString); - } else { - performed = await this.processSingleBatchPull(fileOrPath, path, name, isString); - } - if (performed) results.success++; + const outcome = op === 'push' + ? await this.processSingleBatchPush(fileOrPath, path, name, isString) + : await this.processSingleBatchPull(fileOrPath, path, name, isString); + + if (outcome === 'done') results.success++; + else if (outcome === 'conflict') results.conflicts++; } catch (e) { logger.error(`Failed to ${op} ${path}:`, e); results.failed++; @@ -369,16 +371,19 @@ export class SyncManager { } await this.saveSettings(); - this.notifyBatchResult(op, results.success, results.failed); + this.notifyBatchResult(op, results.success, results.failed, results.conflicts); return results; } - private notifyBatchResult(op: 'push' | 'pull', success: number, failed: number): void { + private notifyBatchResult(op: 'push' | 'pull', success: number, failed: number, conflicts: number): void { const opName = op === 'push' ? 'Pushed' : 'Pulled'; if (success > 0) { new Notice(`${opName} ${success} file(s) to ${this.serviceName}`); } + if (conflicts > 0) { + new Notice(`Skipped ${conflicts} file(s) with conflicting changes on both sides. Push or pull each one individually to resolve.`, 8000); + } if (failed > 0) { new Notice(`Failed to ${op} ${failed} file(s). Check console for details.`); } @@ -421,13 +426,13 @@ export class SyncManager { } } - private async processSingleBatchPush(fileOrPath: TFile | string, path: string, name: string, isString: boolean): Promise { + private async processSingleBatchPush(fileOrPath: TFile | string, path: string, name: string, isString: boolean): Promise { if (!await this.checkFileExists(path, isString)) throw new Error('File no longer exists'); // Symbolic link handling: real → push as a symlink (GitHub), skip → ignore. const symlinkTarget = readLocalSymlinkTarget(this.app, path); if (symlinkTarget !== null && await this.handleSymlinkPush({ path, name }, symlinkTarget, true)) { - return getEffectiveSymlinkHandling(this.settings) !== 'skip'; + return getEffectiveSymlinkHandling(this.settings) !== 'skip' ? 'done' : 'unchanged'; } const content = await this.getFileContent(fileOrPath); @@ -438,26 +443,36 @@ export class SyncManager { const renamedFrom = this.detectRename(fileOrPath); if (renamedFrom) { await this.handleRename(fileOrPath, renamedFrom, content); - return true; + return 'done'; } } const remote = await this.gitService.getFile(repoPath, this.settings.branch); // Don't convert a remote symlink into a regular file. - if (remote.isSymlink) return false; + if (remote.isSymlink) return 'unchanged'; // Skip if already in sync if (remote.sha && this.contentsEqual(content, remote.content)) { await this.updateMetadata(path, remote.sha); - return false; + return 'unchanged'; + } + + // Same conflict check as the single-file flow: if the remote has moved on + // from what we last synced, overwriting it here would silently discard + // whatever changed on the remote. Skip it instead of force-pushing so the + // batch action can't quietly clobber changes the way a single push would + // stop and ask about via SyncConflictModal. + const lastSynced = this.settings.syncMetadata[path]; + if (remote.sha && lastSynced && remote.sha !== lastSynced.lastSyncedSha) { + return 'conflict'; } await this.performPush({ path, name }, content, remote.sha || undefined, true); - return true; + return 'done'; } - private async processSingleBatchPull(fileOrPath: TFile | string, path: string, name: string, isString: boolean): Promise { + private async processSingleBatchPull(fileOrPath: TFile | string, path: string, name: string, isString: boolean): Promise { const repoPath = this.getNormalizedPath(path); const remote = await this.gitService.getFile(repoPath, this.settings.branch); if (!remote.sha) throw new Error('File not found in remote'); @@ -467,12 +482,18 @@ export class SyncManager { const localContent = await this.getFileContent(fileOrPath); if (this.contentsEqual(localContent, remote.content)) { await this.updateMetadata(path, remote.sha); - return false; + return 'unchanged'; + } + + // Same conflict check as the single-file flow (see processSingleBatchPush). + const lastSynced = this.settings.syncMetadata[path]; + if (lastSynced && remote.sha !== lastSynced.lastSyncedSha) { + return 'conflict'; } } const fileRep = typeof fileOrPath === 'string' ? { path, name } : fileOrPath; await this.performPull(fileRep, remote.content, remote.sha, true, this.symlinkPullTarget(remote)); - return true; + return 'done'; } } diff --git a/src/ui/SyncStatusView.ts b/src/ui/SyncStatusView.ts index e522a39..44f260e 100644 --- a/src/ui/SyncStatusView.ts +++ b/src/ui/SyncStatusView.ts @@ -225,7 +225,7 @@ export class SyncStatusView extends ItemView { // ── Single-file operations ────────────────────────────────────── private async handleLocalDelete(fileStatus: FileStatus): Promise { - const confirmed = await this.showConfirmDialog(`Delete local file "${fileStatus.path}"?`); + const confirmed = await this.showConfirmDialog(`Delete local file "${fileStatus.path}"? Handled per your vault's "Deleted files" setting.`); if (!confirmed) return; try { if (fileStatus.file) { @@ -631,10 +631,19 @@ export class SyncStatusView extends ItemView { } private async confirmDeletion(localCount: number, remoteCount: number): Promise { + // Local deletes go through Obsidian's own trash handling, whose actual + // destination (vault .trash/, OS trash, or permanent) depends on the + // user's "Deleted files" setting — not something this plugin can read. + // So local wording defers to that setting rather than promising + // recoverability; remote deletes are unconditionally permanent. let msg = ''; - if (localCount > 0 && remoteCount > 0) msg = `Delete ${localCount} local + ${remoteCount} remote file(s)? Cannot be undone.`; - else if (localCount > 0) msg = `Delete ${localCount} local file(s)? Cannot be undone.`; - else msg = `Delete ${remoteCount} remote file(s)? Cannot be undone.`; + if (localCount > 0 && remoteCount > 0) { + msg = `Delete ${localCount} local file(s) (per your vault's trash setting) and ${remoteCount} remote file(s) (cannot be undone)?`; + } else if (localCount > 0) { + msg = `Delete ${localCount} local file(s)? They'll be handled per your vault's "Deleted files" setting.`; + } else { + msg = `Delete ${remoteCount} remote file(s)? This cannot be undone.`; + } return this.showConfirmDialog(msg); } diff --git a/tests/logic/sync-manager-batch.test.ts b/tests/logic/sync-manager-batch.test.ts index 8bbec82..dd7b3f0 100644 --- a/tests/logic/sync-manager-batch.test.ts +++ b/tests/logic/sync-manager-batch.test.ts @@ -151,6 +151,62 @@ describe('SyncManager Batch Operations', () => { }); }); + describe('batch conflict detection', () => { + it('should skip (not overwrite) a push when the remote has moved on since last sync', async () => { + const path = 'conflicted.md'; + mockSettings.syncMetadata = { + [path]: { lastSyncedSha: 'sha-at-last-sync', lastSyncedAt: 0, lastKnownPath: path } + }; + const adapter = mockApp.vault.adapter as Mocked; + + vi.mocked(adapter.exists).mockResolvedValue(true); + vi.mocked(adapter.read).mockResolvedValue('local edit'); + // Remote sha differs from what we last synced, and content differs too. + vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'remote edit', sha: 'sha-changed-on-remote' }); + + const results = await manager.pushAllFiles([path]); + + expect(results.success).toBe(0); + expect(results.conflicts).toBe(1); + expect(results.failed).toBe(0); + expect(mockGitService.pushFile).not.toHaveBeenCalled(); + }); + + it('should skip (not overwrite) a pull when the local file has diverged since last sync', async () => { + const path = 'conflicted.md'; + mockSettings.syncMetadata = { + [path]: { lastSyncedSha: 'sha-at-last-sync', lastSyncedAt: 0, lastKnownPath: path } + }; + const adapter = mockApp.vault.adapter as Mocked; + + vi.mocked(adapter.exists).mockResolvedValue(true); + vi.mocked(adapter.read).mockResolvedValue('local edit'); + vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'remote edit', sha: 'sha-changed-on-remote' }); + + const results = await manager.pullAllFiles([path]); + + expect(results.success).toBe(0); + expect(results.conflicts).toBe(1); + expect(results.failed).toBe(0); + expect(mockApp.vault.adapter.write).not.toHaveBeenCalled(); + }); + + it('should still push normally when there is no prior sync metadata (not a conflict)', async () => { + const path = 'new-file.md'; + const adapter = mockApp.vault.adapter as Mocked; + + vi.mocked(adapter.exists).mockResolvedValue(true); + vi.mocked(adapter.read).mockResolvedValue('local content'); + vi.mocked(mockGitService.getFile).mockResolvedValue({ content: 'remote content', sha: 'some-sha' }); + vi.mocked(mockGitService.pushFile).mockResolvedValue({ path, sha: 'new-sha' }); + + const results = await manager.pushAllFiles([path]); + + expect(results.success).toBe(1); + expect(results.conflicts).toBe(0); + }); + }); + describe('batch push with rename detection', () => { it('should detect and handle rename during batch push', async () => { const oldPath = 'old.md';