mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-09-27 14:04:08 -03:00
fix(sidebar): verify folder deletion against the backend
The sidebar derives "empty folder" from the models-only list, which omits models flagged `exclude: true`, while the delete guard walks the folder on disk and refuses on any weight file. A folder whose models are all excluded therefore looked empty, offered the confirmation, and then failed with "still contains models". The delete modal still opens on that prediction, but is now corrected by a dry run of the very delete the user is about to confirm, so the button state cannot contradict the backend. The confirm button stays disabled while the check runs, and a late answer is discarded once the modal is dismissed or retargeted. The dry run also covers weight files no scanner indexes (a lora folder holding only a `.gguf`, say) and files that appeared after the last scan. `_collect_folder_manifest()` now reports `excluded_model_count`, and the refusal names the excluded models, so the message explains the mismatch instead of reading like a bug. Locale files carry the sync placeholders in this commit; the translations follow.
This commit is contained in:
@@ -631,24 +631,38 @@ describe('SidebarManager folder deletion', () => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it('opens the confirm state for a folder whose subtree holds no models', () => {
|
||||
const manager = createManager(createApiClient());
|
||||
it('opens the confirm state for a folder whose subtree holds no models', async () => {
|
||||
const apiClient = createApiClient();
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
manager.showDeleteFolderModal('empty');
|
||||
await manager.showDeleteFolderModal('empty');
|
||||
|
||||
const modal = document.getElementById('deleteFolderModal');
|
||||
expect(modal.dataset.state).toBe('confirm');
|
||||
expect(confirmBtn().style.display).toBe('');
|
||||
expect(confirmBtn().disabled).toBe(false);
|
||||
expect(manager._pendingDeleteFolderPath).toBe('empty');
|
||||
expect(modalManager.showModal).toHaveBeenCalledWith('deleteFolderModal');
|
||||
// The prediction is confirmed against the real guard before the user can
|
||||
// act on it.
|
||||
expect(apiClient.deleteFolder).toHaveBeenCalledWith(
|
||||
'/models/loras/empty', { dryRun: true }
|
||||
);
|
||||
});
|
||||
|
||||
it('explains the refusal when the subtree still holds models', () => {
|
||||
const manager = createManager(createApiClient());
|
||||
it('explains the refusal when the subtree still holds models', async () => {
|
||||
const conflict = Object.assign(new Error('still contains models'), {
|
||||
code: 'not_empty',
|
||||
manifest: { model_count: 2, excluded_model_count: 0 },
|
||||
});
|
||||
const apiClient = createApiClient({
|
||||
deleteFolder: vi.fn().mockRejectedValue(conflict),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
manager.showDeleteFolderModal('full');
|
||||
await manager.showDeleteFolderModal('full');
|
||||
|
||||
const modal = document.getElementById('deleteFolderModal');
|
||||
expect(modal.dataset.state).toBe('blocked');
|
||||
@@ -656,17 +670,134 @@ describe('SidebarManager folder deletion', () => {
|
||||
expect(manager._pendingDeleteFolderPath).toBeNull();
|
||||
});
|
||||
|
||||
it('treats an unknown folder as model-free when the models-only set is missing', () => {
|
||||
// nonEmptyFolders is null outside the include-empty tree; the server still
|
||||
// refuses a non-empty folder, so the client falls back to the confirm state.
|
||||
it('treats an unknown folder as model-free when the models-only set is missing', async () => {
|
||||
// nonEmptyFolders is null outside the include-empty tree; the dry run is
|
||||
// what actually decides, so the prediction is only a starting point.
|
||||
const manager = createManager(createApiClient());
|
||||
manager.nonEmptyFolders = null;
|
||||
|
||||
manager.showDeleteFolderModal('empty');
|
||||
await manager.showDeleteFolderModal('empty');
|
||||
|
||||
expect(document.getElementById('deleteFolderModal').dataset.state).toBe('confirm');
|
||||
});
|
||||
|
||||
it('blocks a folder the tree shows as empty when only excluded models live there', async () => {
|
||||
// The reported mismatch: excluded models are absent from the models-only
|
||||
// set (so the node dims as empty), yet they are real weight files on disk
|
||||
// and the delete guard refuses to cascade over them.
|
||||
const conflict = Object.assign(
|
||||
new Error('Folder still contains 3 model file(s), all excluded from the library'),
|
||||
{ code: 'not_empty', manifest: { model_count: 3, excluded_model_count: 3 } }
|
||||
);
|
||||
const apiClient = createApiClient({
|
||||
deleteFolder: vi.fn().mockRejectedValue(conflict),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
await manager.showDeleteFolderModal('Flux.1 D/test');
|
||||
|
||||
const modal = document.getElementById('deleteFolderModal');
|
||||
expect(modal.dataset.state).toBe('blocked');
|
||||
expect(confirmBtn().style.display).toBe('none');
|
||||
expect(manager._pendingDeleteFolderPath).toBeNull();
|
||||
// The message names the excluded models instead of contradicting the tree.
|
||||
expect(modal.querySelector('[data-role="message"]').textContent)
|
||||
.toContain('excluded from the library');
|
||||
expect(apiClient.deleteFolder).toHaveBeenCalledWith(
|
||||
'/models/loras/Flux.1 D/test', { dryRun: true }
|
||||
);
|
||||
});
|
||||
|
||||
it('reports how many model files block the delete when some are excluded', async () => {
|
||||
const conflict = Object.assign(new Error('still contains models'), {
|
||||
code: 'not_empty',
|
||||
manifest: { model_count: 4, excluded_model_count: 1 },
|
||||
});
|
||||
const apiClient = createApiClient({
|
||||
deleteFolder: vi.fn().mockRejectedValue(conflict),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
await manager.showDeleteFolderModal('mixed');
|
||||
|
||||
expect(
|
||||
document.getElementById('deleteFolderModal')
|
||||
.querySelector('[data-role="message"]').textContent
|
||||
).toContain('4 model file(s)');
|
||||
});
|
||||
|
||||
it('blocks the delete while a staged delete is still pending', async () => {
|
||||
const busy = Object.assign(new Error('staged delete pending'), { code: 'busy' });
|
||||
const apiClient = createApiClient({
|
||||
deleteFolder: vi.fn().mockRejectedValue(busy),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
await manager.showDeleteFolderModal('empty');
|
||||
|
||||
const modal = document.getElementById('deleteFolderModal');
|
||||
expect(modal.dataset.state).toBe('busy');
|
||||
expect(confirmBtn().style.display).toBe('none');
|
||||
});
|
||||
|
||||
it('keeps the confirm button disabled until the check settles', async () => {
|
||||
let release;
|
||||
const apiClient = createApiClient({
|
||||
fetchModelRoots: vi.fn(() => new Promise((resolve) => { release = resolve; })),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
const pending = manager.showDeleteFolderModal('empty');
|
||||
expect(confirmBtn().disabled).toBe(true);
|
||||
|
||||
release({ roots: ['/models/loras'] });
|
||||
await pending;
|
||||
|
||||
expect(confirmBtn().disabled).toBe(false);
|
||||
expect(document.getElementById('deleteFolderModal').dataset.state).toBe('confirm');
|
||||
});
|
||||
|
||||
it('ignores a dry-run answer that lands after the modal was dismissed', async () => {
|
||||
let rejectProbe;
|
||||
const apiClient = createApiClient({
|
||||
deleteFolder: vi.fn(() => new Promise((_resolve, reject) => { rejectProbe = reject; })),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
const pending = manager.showDeleteFolderModal('empty');
|
||||
expect(document.getElementById('deleteFolderModal').dataset.state).toBe('confirm');
|
||||
|
||||
await vi.waitFor(() => expect(rejectProbe).toBeTypeOf('function'));
|
||||
|
||||
manager.hideDeleteFolderModal();
|
||||
rejectProbe(Object.assign(new Error('still contains models'), {
|
||||
code: 'not_empty',
|
||||
manifest: { model_count: 1, excluded_model_count: 0 },
|
||||
}));
|
||||
await pending;
|
||||
|
||||
expect(document.getElementById('deleteFolderModal').dataset.state).toBe('confirm');
|
||||
});
|
||||
|
||||
it('falls back to the tree prediction when the check fails for another reason', async () => {
|
||||
const apiClient = createApiClient({
|
||||
deleteFolder: vi.fn().mockRejectedValue(new Error('network down')),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.nonEmptyFolders = new Set(['', 'full']);
|
||||
|
||||
await manager.showDeleteFolderModal('empty');
|
||||
|
||||
const modal = document.getElementById('deleteFolderModal');
|
||||
expect(modal.dataset.state).toBe('confirm');
|
||||
expect(confirmBtn().disabled).toBe(false);
|
||||
});
|
||||
|
||||
it('deletes the folder and offers the undo affordance for an empty one', async () => {
|
||||
const apiClient = createApiClient();
|
||||
const manager = createManager(apiClient);
|
||||
@@ -732,6 +863,25 @@ describe('SidebarManager folder deletion', () => {
|
||||
expect(manager.refresh).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('includes the model count in the stale-tree toast when the manifest has one', async () => {
|
||||
const conflict = Object.assign(new Error('still contains models'), {
|
||||
code: 'not_empty',
|
||||
manifest: { model_count: 3, excluded_model_count: 3 },
|
||||
});
|
||||
const apiClient = createApiClient({
|
||||
deleteFolder: vi.fn().mockRejectedValue(conflict),
|
||||
});
|
||||
const manager = createManager(apiClient);
|
||||
manager.refresh = vi.fn().mockResolvedValue(undefined);
|
||||
|
||||
const success = await manager._deleteFolder('full');
|
||||
|
||||
expect(success).toBe(false);
|
||||
expect(showToast).toHaveBeenCalledWith(
|
||||
'sidebar.deleteFolderResult.notEmptyWithCount', { count: 3 }, 'warning'
|
||||
);
|
||||
});
|
||||
|
||||
it('surfaces a busy folder with a staged delete', async () => {
|
||||
const busy = Object.assign(new Error('staged delete pending'), { code: 'busy' });
|
||||
const apiClient = createApiClient({
|
||||
|
||||
@@ -11,15 +11,19 @@ from py.services.model_file_service import ModelMoveService
|
||||
|
||||
|
||||
class FakeScanner:
|
||||
def __init__(self, roots: List[Path]) -> None:
|
||||
def __init__(self, roots: List[Path], excluded: List[str] | None = None) -> None:
|
||||
self._roots = [str(root) for root in roots]
|
||||
self.known_folders: List[str] = []
|
||||
self.removed_folders: List[str] = []
|
||||
self.renamed_folders: List[tuple] = []
|
||||
self._excluded = list(excluded or [])
|
||||
|
||||
def get_model_roots(self) -> List[str]:
|
||||
return list(self._roots)
|
||||
|
||||
def get_excluded_models(self) -> List[str]:
|
||||
return list(self._excluded)
|
||||
|
||||
async def add_known_folder(self, folder: str) -> None:
|
||||
self.known_folders.append(folder)
|
||||
|
||||
@@ -30,6 +34,12 @@ class FakeScanner:
|
||||
self.renamed_folders.append((previous, current, kwargs))
|
||||
|
||||
|
||||
class ScannerWithoutExcludedAccessor(FakeScanner):
|
||||
"""Scanner stand-in predating ``get_excluded_models()``."""
|
||||
|
||||
get_excluded_models = None # type: ignore[assignment]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_folder_creates_directory_and_registers_it(tmp_path: Path):
|
||||
scanner = FakeScanner([tmp_path])
|
||||
@@ -149,11 +159,90 @@ async def test_delete_folder_refuses_when_models_live_below(tmp_path: Path):
|
||||
assert result["success"] is False
|
||||
assert result["code"] == "not_empty"
|
||||
assert result["manifest"]["model_count"] == 1
|
||||
assert result["manifest"]["excluded_model_count"] == 0
|
||||
assert target.exists()
|
||||
assert model_file.exists()
|
||||
assert scanner.removed_folders == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_folder_manifest_marks_models_excluded_from_the_library(tmp_path: Path):
|
||||
"""Excluded models are invisible to the model lists but still block the
|
||||
cascade, so the manifest has to say so — the folder sidebar otherwise shows
|
||||
the folder as empty and the refusal reads as a bug."""
|
||||
target = _make_nested(tmp_path)
|
||||
model_file = target / "hidden.safetensors"
|
||||
model_file.write_text("weights", encoding="utf-8")
|
||||
scanner = FakeScanner([tmp_path], excluded=[str(model_file)])
|
||||
service = ModelMoveService(scanner, "lora")
|
||||
|
||||
result = await service.delete_folder(str(target))
|
||||
|
||||
assert result["success"] is False
|
||||
assert result["code"] == "not_empty"
|
||||
assert result["manifest"]["model_count"] == 1
|
||||
assert result["manifest"]["excluded_model_count"] == 1
|
||||
assert "excluded" in result["error"]
|
||||
assert model_file.exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_folder_error_splits_excluded_from_visible_models(tmp_path: Path):
|
||||
target = _make_nested(tmp_path)
|
||||
visible = target / "visible.safetensors"
|
||||
hidden = target / "hidden.safetensors"
|
||||
visible.write_text("weights", encoding="utf-8")
|
||||
hidden.write_text("weights", encoding="utf-8")
|
||||
scanner = FakeScanner([tmp_path], excluded=[str(hidden)])
|
||||
service = ModelMoveService(scanner, "lora")
|
||||
|
||||
result = await service.delete_folder(str(target))
|
||||
|
||||
assert result["manifest"]["model_count"] == 2
|
||||
assert result["manifest"]["excluded_model_count"] == 1
|
||||
assert "1 of them excluded" in result["error"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_folder_does_not_claim_excluded_for_visible_models(tmp_path: Path):
|
||||
target = _make_nested(tmp_path)
|
||||
(target / "model.safetensors").write_text("weights", encoding="utf-8")
|
||||
# An excluded model elsewhere in the library must not be attributed here.
|
||||
scanner = FakeScanner([tmp_path], excluded=[str(tmp_path / "other" / "other.safetensors")])
|
||||
service = ModelMoveService(scanner, "lora")
|
||||
|
||||
result = await service.delete_folder(str(target))
|
||||
|
||||
assert result["manifest"]["excluded_model_count"] == 0
|
||||
assert "excluded" not in result["error"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_folder_works_without_the_excluded_accessor(tmp_path: Path):
|
||||
target = _make_nested(tmp_path)
|
||||
(target / "model.safetensors").write_text("weights", encoding="utf-8")
|
||||
scanner = ScannerWithoutExcludedAccessor([tmp_path])
|
||||
service = ModelMoveService(scanner, "lora")
|
||||
|
||||
result = await service.delete_folder(str(target))
|
||||
|
||||
assert result["success"] is False
|
||||
assert result["manifest"]["excluded_model_count"] == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_folder_success_manifest_carries_excluded_model_count(tmp_path: Path):
|
||||
target = _make_nested(tmp_path)
|
||||
(target / "leftover.webp").write_text("preview", encoding="utf-8")
|
||||
scanner = FakeScanner([tmp_path], excluded=[str(tmp_path / "elsewhere.safetensors")])
|
||||
service = ModelMoveService(scanner, "lora")
|
||||
|
||||
result = await service.delete_folder(str(target), dry_run=True)
|
||||
|
||||
assert result["success"] is True
|
||||
assert result["excluded_model_count"] == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_folder_refuses_while_a_staged_delete_is_pending(tmp_path: Path):
|
||||
target = _make_nested(tmp_path)
|
||||
|
||||
Reference in New Issue
Block a user