fix: address ObsidianReviewBot code review issues
- Fix template literal type issue in notifications.ts by adding explicit String() coercion for args.recursive - Replace Vault.trash() with FileManager.trashFile() to respect user's configured deletion preferences (system trash or .trash/) - Remove unused trash() method from IVaultAdapter and VaultAdapter - Update tests to reflect new deletion behavior
This commit is contained in:
@@ -33,9 +33,6 @@ export interface IVaultAdapter {
|
||||
|
||||
// File modification
|
||||
modify(file: TFile, data: string): Promise<void>;
|
||||
|
||||
// File deletion (respects Obsidian trash settings)
|
||||
trash(file: TAbstractFile, system: boolean): Promise<void>;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -42,8 +42,4 @@ export class VaultAdapter implements IVaultAdapter {
|
||||
async modify(file: TFile, data: string): Promise<void> {
|
||||
await this.vault.modify(file, data);
|
||||
}
|
||||
|
||||
async trash(file: TAbstractFile, system: boolean): Promise<void> {
|
||||
await this.vault.trash(file, system);
|
||||
}
|
||||
}
|
||||
@@ -587,7 +587,8 @@ export class NoteTools {
|
||||
// Dry run - just return what would happen
|
||||
if (dryRun) {
|
||||
if (soft) {
|
||||
destination = `.trash/${file.name}`;
|
||||
// Destination depends on user's configured deletion preference
|
||||
destination = 'trash';
|
||||
}
|
||||
|
||||
const result: DeleteNoteResult = {
|
||||
@@ -603,14 +604,13 @@ export class NoteTools {
|
||||
};
|
||||
}
|
||||
|
||||
// Perform actual deletion
|
||||
// Perform actual deletion using user's preferred trash settings
|
||||
// FileManager.trashFile() respects the user's configured deletion preference
|
||||
// (system trash or .trash/ folder) as set in Obsidian settings
|
||||
await this.fileManager.trashFile(file);
|
||||
if (soft) {
|
||||
// Move to trash using Obsidian's trash method
|
||||
await this.vault.trash(file, true);
|
||||
destination = `.trash/${file.name}`;
|
||||
} else {
|
||||
// Delete using user's preferred trash settings (system trash or .trash/ folder)
|
||||
await this.fileManager.trashFile(file);
|
||||
// For soft delete, indicate the file was moved to trash (location depends on user settings)
|
||||
destination = 'trash';
|
||||
}
|
||||
|
||||
const result: DeleteNoteResult = {
|
||||
|
||||
@@ -163,7 +163,7 @@ export class NotificationManager {
|
||||
keyParams.push(`folder: "${this.truncateString(args.folder, 30)}"`);
|
||||
}
|
||||
if (args.recursive !== undefined) {
|
||||
keyParams.push(`recursive: ${args.recursive}`);
|
||||
keyParams.push(`recursive: ${String(args.recursive)}`);
|
||||
}
|
||||
|
||||
// If no key params, show first 50 chars of JSON
|
||||
|
||||
@@ -436,15 +436,16 @@ describe('NoteTools', () => {
|
||||
const mockFile = createMockTFile('test.md');
|
||||
|
||||
(PathUtils.resolveFile as jest.Mock).mockReturnValue(mockFile);
|
||||
mockVault.trash = jest.fn().mockResolvedValue(undefined);
|
||||
mockFileManager.trashFile = jest.fn().mockResolvedValue(undefined);
|
||||
|
||||
const result = await noteTools.deleteNote('test.md', true, false);
|
||||
|
||||
expect(result.isError).toBeUndefined();
|
||||
expect(mockVault.trash).toHaveBeenCalledWith(mockFile, true);
|
||||
expect(mockFileManager.trashFile).toHaveBeenCalledWith(mockFile);
|
||||
const parsed = JSON.parse(result.content[0].text);
|
||||
expect(parsed.deleted).toBe(true);
|
||||
expect(parsed.soft).toBe(true);
|
||||
expect(parsed.destination).toBe('trash');
|
||||
});
|
||||
|
||||
it('should permanently delete note', async () => {
|
||||
@@ -466,6 +467,7 @@ describe('NoteTools', () => {
|
||||
const mockFile = createMockTFile('test.md');
|
||||
|
||||
(PathUtils.resolveFile as jest.Mock).mockReturnValue(mockFile);
|
||||
mockFileManager.trashFile = jest.fn().mockResolvedValue(undefined);
|
||||
|
||||
const result = await noteTools.deleteNote('test.md', true, true);
|
||||
|
||||
@@ -473,7 +475,8 @@ describe('NoteTools', () => {
|
||||
const parsed = JSON.parse(result.content[0].text);
|
||||
expect(parsed.deleted).toBe(false);
|
||||
expect(parsed.dryRun).toBe(true);
|
||||
expect(mockVault.trash).not.toHaveBeenCalled();
|
||||
expect(parsed.destination).toBe('trash');
|
||||
expect(mockFileManager.trashFile).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should return error if file not found', async () => {
|
||||
@@ -500,7 +503,7 @@ describe('NoteTools', () => {
|
||||
const mockFile = createMockTFile('test.md');
|
||||
|
||||
(PathUtils.resolveFile as jest.Mock).mockReturnValue(mockFile);
|
||||
mockVault.trash = jest.fn().mockRejectedValue(new Error('Cannot delete'));
|
||||
mockFileManager.trashFile = jest.fn().mockRejectedValue(new Error('Cannot delete'));
|
||||
|
||||
const result = await noteTools.deleteNote('test.md');
|
||||
|
||||
|
||||
Reference in New Issue
Block a user