fix: Unify folder checks and report bulk skip counts after conflict merge

Align RuleManagerV2 preview with move runtime by using folderExists
(adapter.exists) instead of getAbstractFileByPath. Extend FileMoveResult
with skip reasons, centralize skip-outcome helpers, and surface
missing-folder and conflict skips in bulk, periodic, and preview
completion notices even when no files were moved.
This commit is contained in:
Lars Bücker 2026-07-10 22:14:37 +02:00
parent 9752cf5e1d
commit 54af21b40f
10 changed files with 314 additions and 86 deletions

View file

@ -21,6 +21,12 @@ describe('isNoteMoveConflictSkipOutcome', () => {
});
it('returns false for successful moves', () => {
expect(isNoteMoveConflictSkipOutcome({ moved: true })).toBe(false);
expect(
isNoteMoveConflictSkipOutcome({
moved: true,
newPath: 'Dest/note.md',
targetFolder: 'Dest',
})
).toBe(false);
});
});

View file

@ -1,13 +1,7 @@
type NoteMoveConflictSkipCheckOutcome =
| { moved: true }
| { moved: false; reason: 'skip' | 'unchanged' | 'cached_skip' };
/** True when a move was not performed because of a naming conflict skip. */
export function isNoteMoveConflictSkipOutcome(
outcome: NoteMoveConflictSkipCheckOutcome
): boolean {
return (
!outcome.moved &&
(outcome.reason === 'skip' || outcome.reason === 'cached_skip')
);
}
export {
isNoteMoveConflictSkipOutcome,
isConflictSkipFileMoveOutcome,
isMissingFolderSkipOutcome,
formatMoveSkippedDetail,
fileMoveResultFromConflictOutcome,
} from './note-move-skip-outcome';

View file

@ -0,0 +1,98 @@
import { describe, it, expect } from 'vitest';
import {
formatMoveSkippedDetail,
fileMoveResultFromConflictOutcome,
isConflictSkipFileMoveOutcome,
isMissingFolderSkipOutcome,
isNoteMoveConflictSkipOutcome,
} from './note-move-skip-outcome';
describe('formatMoveSkippedDetail', () => {
it('returns empty string when nothing was skipped', () => {
expect(formatMoveSkippedDetail({ missingFolder: 0, conflict: 0 })).toBe('');
});
it('formats missing-folder skips only', () => {
expect(formatMoveSkippedDetail({ missingFolder: 3, conflict: 0 })).toBe(
' 3 skipped (destination folder missing).'
);
});
it('formats conflict skips only', () => {
expect(formatMoveSkippedDetail({ missingFolder: 0, conflict: 2 })).toBe(
' 2 skipped due to conflicts.'
);
});
it('formats combined skip counts', () => {
expect(formatMoveSkippedDetail({ missingFolder: 1, conflict: 4 })).toBe(
' 1 skipped (destination folder missing), 4 skipped due to conflicts.'
);
});
});
describe('isMissingFolderSkipOutcome', () => {
it('detects missing destination folder skips', () => {
expect(
isMissingFolderSkipOutcome({
moved: false,
skipReason: 'missing_destination_folder',
})
).toBe(true);
expect(isMissingFolderSkipOutcome({ moved: false })).toBe(false);
expect(isMissingFolderSkipOutcome({ moved: true, targetFolder: 'A' })).toBe(
false
);
});
});
describe('isConflictSkipFileMoveOutcome', () => {
it('detects conflict and cached conflict skips', () => {
expect(
isConflictSkipFileMoveOutcome({
moved: false,
skipReason: 'conflict_skip',
})
).toBe(true);
expect(
isConflictSkipFileMoveOutcome({
moved: false,
skipReason: 'cached_conflict_skip',
})
).toBe(true);
expect(
isConflictSkipFileMoveOutcome({
moved: false,
skipReason: 'missing_destination_folder',
})
).toBe(false);
});
});
describe('isNoteMoveConflictSkipOutcome', () => {
it('detects conflict skip reasons from execute outcome', () => {
expect(
isNoteMoveConflictSkipOutcome({ moved: false, reason: 'skip' })
).toBe(true);
expect(
isNoteMoveConflictSkipOutcome({ moved: false, reason: 'cached_skip' })
).toBe(true);
expect(
isNoteMoveConflictSkipOutcome({ moved: false, reason: 'unchanged' })
).toBe(false);
});
});
describe('fileMoveResultFromConflictOutcome', () => {
it('maps conflict skip reasons to FileMoveResult', () => {
expect(
fileMoveResultFromConflictOutcome({ moved: false, reason: 'skip' })
).toEqual({ moved: false, skipReason: 'conflict_skip' });
expect(
fileMoveResultFromConflictOutcome({ moved: false, reason: 'cached_skip' })
).toEqual({ moved: false, skipReason: 'cached_conflict_skip' });
expect(
fileMoveResultFromConflictOutcome({ moved: false, reason: 'unchanged' })
).toEqual({ moved: false });
});
});

View file

@ -0,0 +1,64 @@
import type { FileMoveResult } from '../types/Common';
import type { ExecuteNoteMoveWithConflictResult } from './execute-note-move-with-conflict';
export interface MoveSkippedCounts {
missingFolder: number;
conflict: number;
}
/** True when a move was skipped because the destination folder is missing. */
export function isMissingFolderSkipOutcome(result: FileMoveResult): boolean {
return !result.moved && result.skipReason === 'missing_destination_folder';
}
/** True when a move was skipped because of a naming conflict (or cached skip). */
export function isConflictSkipFileMoveOutcome(result: FileMoveResult): boolean {
return (
!result.moved &&
(result.skipReason === 'conflict_skip' ||
result.skipReason === 'cached_conflict_skip')
);
}
/** True when a move was not performed because of a naming conflict skip. */
export function isNoteMoveConflictSkipOutcome(
outcome: ExecuteNoteMoveWithConflictResult
): boolean {
return (
!outcome.moved &&
(outcome.reason === 'skip' || outcome.reason === 'cached_skip')
);
}
/** Maps a conflict-handling outcome to a {@link FileMoveResult} skip reason. */
export function fileMoveResultFromConflictOutcome(
outcome: ExecuteNoteMoveWithConflictResult
): FileMoveResult {
if (outcome.moved) {
return { moved: true, targetFolder: outcome.targetFolder };
}
if (outcome.reason === 'skip') {
return { moved: false, skipReason: 'conflict_skip' };
}
if (outcome.reason === 'cached_skip') {
return { moved: false, skipReason: 'cached_conflict_skip' };
}
return { moved: false };
}
/** Formats aggregated skip counts for bulk / preview completion notices. */
export function formatMoveSkippedDetail(counts: MoveSkippedCounts): string {
const total = counts.missingFolder + counts.conflict;
if (total === 0) {
return '';
}
const parts: string[] = [];
if (counts.missingFolder > 0) {
parts.push(`${counts.missingFolder} skipped (destination folder missing)`);
}
if (counts.conflict > 0) {
parts.push(`${counts.conflict} skipped due to conflicts`);
}
return ` ${parts.join(', ')}.`;
}

View file

@ -17,6 +17,12 @@ import { persistConflictResolutionStrategy } from '../application/persist-confli
import { getAttachmentMoveSettings } from '../utils/attachment-settings';
import AdvancedNoteMoverPlugin from 'main';
import { type FileMoveResult, type OperationType } from '../types/Common';
import {
fileMoveResultFromConflictOutcome,
formatMoveSkippedDetail,
isConflictSkipFileMoveOutcome,
isMissingFolderSkipOutcome,
} from '../application/note-move-skip-outcome';
import { showSingleFileMoveNotice } from '../utils/single-file-move-notice';
import { MovePreview } from '../types/MovePreview';
import { PreviewModal } from '../modals/PreviewModal';
@ -217,7 +223,10 @@ export class AdvancedNoteMover {
!(await folderExists(app, targetFolder))
) {
this.maybeNotifyMissingFolder(file, targetFolder);
return this.finishFileMove(originalPath, file, { moved: false });
return this.finishFileMove(originalPath, file, {
moved: false,
skipReason: 'missing_destination_folder',
});
}
if (!(await ensureFolderExists(app, targetFolder))) {
@ -245,7 +254,11 @@ export class AdvancedNoteMover {
});
if (!moveOutcome.moved) {
return this.finishFileMove(originalPath, file, { moved: false });
return this.finishFileMove(
originalPath,
file,
fileMoveResultFromConflictOutcome(moveOutcome)
);
}
const moveResult = {
@ -289,6 +302,8 @@ export class AdvancedNoteMover {
);
let successCount = 0;
let errorCount = 0;
let missingFolderSkippedCount = 0;
let conflictSkippedCount = 0;
if (options.operationType === 'periodic') {
await this.plugin.conflictSkipCacheManager.prune(app);
@ -308,6 +323,10 @@ export class AdvancedNoteMover {
);
if (moveResult.moved) {
successCount++;
} else if (isMissingFolderSkipOutcome(moveResult)) {
missingFolderSkippedCount++;
} else if (isConflictSkipFileMoveOutcome(moveResult)) {
conflictSkippedCount++;
}
} catch (error) {
errorCount++;
@ -330,43 +349,63 @@ export class AdvancedNoteMover {
}
}
// Show completion notice
if (successCount > 0 && options.showNotifications) {
if (options.operationType === 'bulk') {
const totalFiles = successCount + errorCount;
const successMessage =
errorCount > 0
? `Moved ${successCount}/${totalFiles} files. ${errorCount} files had errors.`
: `Successfully moved ${successCount} files`;
const skippedDetail = formatMoveSkippedDetail({
missingFolder: missingFolderSkippedCount,
conflict: conflictSkippedCount,
});
const hasSkipOrError =
missingFolderSkippedCount > 0 ||
conflictSkippedCount > 0 ||
errorCount > 0;
NoticeManager.showWithUndo(
'info',
`Bulk Operation: ${successMessage}`,
() => {
void (async () => {
const success =
await this.plugin.historyManager.undoBulkOperation(
bulkOperationId
);
if (success) {
NoticeManager.success(
`Bulk operation undone: ${successCount} files moved back`,
{ duration: NOTIFICATION_CONSTANTS.DURATION_OVERRIDE }
);
} else {
NoticeManager.warning(
`Could not undo all moves. Check individual files in history.`,
{ duration: NOTIFICATION_CONSTANTS.DURATION_OVERRIDE }
);
}
})();
},
SETTINGS_CONSTANTS.UI_TEXTS.UNDO_ALL
);
// Show completion notice when anything happened worth reporting
if (
options.showNotifications &&
(successCount > 0 || hasSkipOrError)
) {
if (options.operationType === 'bulk') {
if (successCount > 0) {
const totalAttempted = successCount + errorCount;
const successMessage =
errorCount > 0
? `Moved ${successCount}/${totalAttempted} files. ${errorCount} files had errors.`
: `Successfully moved ${successCount} files`;
NoticeManager.showWithUndo(
'info',
`Bulk Operation: ${successMessage}${skippedDetail}`,
() => {
void (async () => {
const success =
await this.plugin.historyManager.undoBulkOperation(
bulkOperationId
);
if (success) {
NoticeManager.success(
`Bulk operation undone: ${successCount} files moved back`,
{ duration: NOTIFICATION_CONSTANTS.DURATION_OVERRIDE }
);
} else {
NoticeManager.warning(
`Could not undo all moves. Check individual files in history.`,
{ duration: NOTIFICATION_CONSTANTS.DURATION_OVERRIDE }
);
}
})();
},
SETTINGS_CONSTANTS.UI_TEXTS.UNDO_ALL
);
} else {
NoticeManager.info(
`Bulk Operation: No files moved.${skippedDetail}`
);
}
} else {
NoticeManager.info(
`Periodic movement: Successfully moved ${successCount} files`
);
const periodicMessage =
successCount > 0
? `Periodic movement: Successfully moved ${successCount} files${skippedDetail}`
: `Periodic movement: No files moved.${skippedDetail}`;
NoticeManager.info(periodicMessage);
}
}
} finally {

View file

@ -30,6 +30,9 @@ function makeApp(existingFolders: string[] = []): App {
},
vault: {
read: async () => '',
adapter: {
exists: async (path: string) => folders.has(path),
},
getAbstractFileByPath: (path: string) =>
folders.has(path) ? ({ path } as unknown) : null,
},

View file

@ -5,7 +5,7 @@ import { handleError } from '../utils/Error';
import {
combinePath,
DESTINATION_PATH_BLOCK_REASONS,
formatPath,
folderExists,
normalizeDestinationFolderPath,
} from '../utils/PathUtils';
import { MetadataExtractor } from './MetadataExtractor';
@ -246,7 +246,7 @@ export class RuleManagerV2 {
// When auto-create is disabled and the destination folder does not
// exist yet, the move would be skipped - reflect that in the preview.
if (!createFolder && !this.destinationFolderExists(targetFolder)) {
if (!createFolder && !(await folderExists(this.app, targetFolder))) {
return {
fileName,
currentPath: filePath,
@ -396,21 +396,6 @@ export class RuleManagerV2 {
/**
* Applies the same destination normalization for move and preview so both agree.
*/
/**
* Synchronous existence check for a destination folder, used during preview
* generation. Root always counts as existing.
*/
private destinationFolderExists(folderPath: string): boolean {
if (!folderPath || folderPath === '/' || folderPath === '') {
return true;
}
const formatted = formatPath(folderPath);
if (!formatted) {
return true;
}
return this.app.vault.getAbstractFileByPath(formatted) !== null;
}
private normalizeRenderedDestination(
rendered: string
): { ok: true; path: string } | { ok: false; reason: string } {

View file

@ -9,7 +9,10 @@ import {
folderExists,
} from '../utils/PathUtils';
import { handleError, createError } from '../utils/Error';
import { isNoteMoveConflictSkipOutcome } from '../application/note-move-conflict-skip-outcome';
import {
formatMoveSkippedDetail,
isNoteMoveConflictSkipOutcome,
} from '../application/note-move-skip-outcome';
import { executeNoteMoveWithConflictHandling } from '../application/execute-note-move-with-conflict';
import { persistConflictResolutionStrategy } from '../application/persist-conflict-resolution-strategy';
import { getAttachmentMoveSettings } from '../utils/attachment-settings';
@ -313,22 +316,11 @@ export class PreviewModal extends BaseModal {
this.close();
const skippedDetail = formatMoveSkippedDetail({
missingFolder: missingFolderSkippedCount,
conflict: conflictSkippedCount,
});
const totalSkipped = missingFolderSkippedCount + conflictSkippedCount;
const skippedDetail = (() => {
if (totalSkipped === 0) {
return '';
}
const parts: string[] = [];
if (missingFolderSkippedCount > 0) {
parts.push(
`${missingFolderSkippedCount} skipped (destination folder missing)`
);
}
if (conflictSkippedCount > 0) {
parts.push(`${conflictSkippedCount} skipped due to conflicts`);
}
return ` ${parts.join(', ')}.`;
})();
if (abortCtl.signal.aborted) {
NoticeManager.info(

View file

@ -11,9 +11,15 @@ export type NotificationType =
export type OperationType = 'single' | 'bulk' | 'periodic';
/** Why a rule-based move was skipped without moving the file. */
export type FileMoveSkipReason =
| 'missing_destination_folder'
| 'conflict_skip'
| 'cached_conflict_skip';
/** Result of attempting to move one file via rules. */
export type FileMoveResult =
| { moved: false }
| { moved: false; skipReason?: FileMoveSkipReason }
| { moved: true; targetFolder: string };
/**

View file

@ -1,9 +1,50 @@
import { describe, it, expect } from 'vitest';
import { describe, it, expect, vi } from 'vitest';
import type { App } from 'obsidian';
import {
folderExists,
normalizeDestinationFolderPath,
sanitizePathSegment,
} from './PathUtils';
function makeApp(existsImpl: (path: string) => Promise<boolean>): App {
return {
vault: {
adapter: {
exists: existsImpl,
},
},
} as unknown as App;
}
describe('folderExists', () => {
it('treats root and empty paths as existing', async () => {
const exists = vi.fn().mockResolvedValue(false);
const app = makeApp(exists);
expect(await folderExists(app, '')).toBe(true);
expect(await folderExists(app, '/')).toBe(true);
expect(exists).not.toHaveBeenCalled();
});
it('checks formatted vault-relative paths via adapter.exists', async () => {
const exists = vi
.fn()
.mockImplementation(async (path: string) => path === 'Projects/Notes');
const app = makeApp(exists);
expect(await folderExists(app, '/Projects/Notes/')).toBe(true);
expect(await folderExists(app, 'Projects/Missing')).toBe(false);
expect(exists).toHaveBeenCalledWith('Projects/Notes');
});
it('returns false when adapter.exists throws', async () => {
const exists = vi.fn().mockRejectedValue(new Error('IO error'));
const app = makeApp(exists);
expect(await folderExists(app, 'Projects')).toBe(false);
});
});
describe('sanitizePathSegment', () => {
it('unwraps wikilinks and strips invalid characters', () => {
expect(sanitizePathSegment('[[Client A]]')).toBe('Client A');