mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-10-10 19:42:13 -03:00
fix(sidecars): report over-long migration paths and fix root relocation (#1142)
- Surface Windows WinError 206 / ENAMETOOLONG during sidecar migration as an actionable per-model error instead of a bare OSError - Offer root relocation based on the resolved sidecar root, so moving off (or back to) the default storage directory relocates existing sidecars - Hint in the migration confirm dialog when the destination is the default location, so a custom folder can be set before migrating
This commit is contained in:
@@ -117,6 +117,7 @@ const appendMigrationModal = () => {
|
||||
<h2 data-role="title"></h2>
|
||||
<p data-role="message"></p>
|
||||
<p data-role="destination" style="display:none"></p>
|
||||
<p data-role="default-location-hint" style="display:none"></p>
|
||||
<button data-action="confirm-sidecar-migration"></button>
|
||||
<button data-action="cancel-sidecar-migration"></button>`;
|
||||
document.body.appendChild(modal);
|
||||
@@ -130,6 +131,26 @@ const mockFetchOk = (payload = { success: true }) => {
|
||||
});
|
||||
};
|
||||
|
||||
// Fetch mock where the GET /api/lm/settings refresh reports ``resolvedRoot``
|
||||
// as the new resolved sidecar root; every other call succeeds generically.
|
||||
const mockFetchWithResolvedRoot = (resolvedRoot) => {
|
||||
global.fetch = vi.fn((url, options) => {
|
||||
if (url === '/api/lm/settings' && !options) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
json: () => Promise.resolve({
|
||||
success: true,
|
||||
settings: { sidecar_storage_root: resolvedRoot },
|
||||
}),
|
||||
});
|
||||
}
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
json: () => Promise.resolve({ success: true }),
|
||||
});
|
||||
});
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
document.body.innerHTML = '';
|
||||
vi.clearAllMocks();
|
||||
@@ -255,6 +276,53 @@ describe('SettingsManager sidecar storage', () => {
|
||||
await changePromise;
|
||||
});
|
||||
|
||||
it('hints at setting a custom folder first when migrating to the default location', async () => {
|
||||
const manager = createManager();
|
||||
const { select } = appendSidecarControls();
|
||||
const modal = appendMigrationModal();
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'alongside',
|
||||
sidecar_storage_root: '/settings/sidecars',
|
||||
sidecar_storage_root_is_default: true,
|
||||
};
|
||||
manager._loadedSidecarStorageMode = 'alongside';
|
||||
select.value = 'centralized';
|
||||
mockFetchOk();
|
||||
|
||||
const changePromise = manager.handleSidecarStorageModeChange();
|
||||
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
|
||||
|
||||
const hint = modal.querySelector('[data-role="default-location-hint"]');
|
||||
expect(hint.style.display).toBe('block');
|
||||
expect(hint.textContent).toContain('default location');
|
||||
|
||||
modal.querySelector('[data-action="cancel-sidecar-migration"]').click();
|
||||
await changePromise;
|
||||
});
|
||||
|
||||
it('hides the default-location hint for custom roots and other directions', async () => {
|
||||
const manager = createManager();
|
||||
const { select } = appendSidecarControls();
|
||||
const modal = appendMigrationModal();
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'alongside',
|
||||
sidecar_storage_root: '/data/sidecars',
|
||||
sidecar_storage_root_is_default: false,
|
||||
};
|
||||
manager._loadedSidecarStorageMode = 'alongside';
|
||||
select.value = 'centralized';
|
||||
mockFetchOk();
|
||||
|
||||
const changePromise = manager.handleSidecarStorageModeChange();
|
||||
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
|
||||
|
||||
const hint = modal.querySelector('[data-role="default-location-hint"]');
|
||||
expect(hint.style.display).toBe('none');
|
||||
|
||||
modal.querySelector('[data-action="cancel-sidecar-migration"]').click();
|
||||
await changePromise;
|
||||
});
|
||||
|
||||
it('shows a deferred notice and skips migration when the user cancels', async () => { const manager = createManager();
|
||||
const { select } = appendSidecarControls();
|
||||
const modal = appendMigrationModal();
|
||||
@@ -311,14 +379,20 @@ describe('SettingsManager sidecar storage', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('handleSidecarStoragePathChange', () => { it('offers root relocation when the path changes in centralized mode', async () => {
|
||||
describe('handleSidecarStoragePathChange', () => {
|
||||
it('offers root relocation when the path changes in centralized mode', async () => {
|
||||
const manager = createManager();
|
||||
const { pathInput } = appendSidecarControls();
|
||||
const modal = appendMigrationModal();
|
||||
state.global.settings = { sidecar_storage_mode: 'centralized', sidecar_storage_path: '/old/root' };
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'centralized',
|
||||
sidecar_storage_path: '/old/root',
|
||||
sidecar_storage_root: '/old/root',
|
||||
};
|
||||
manager._loadedSidecarStoragePath = '/old/root';
|
||||
manager._loadedSidecarStorageRoot = '/old/root';
|
||||
pathInput.value = '/new/root';
|
||||
mockFetchOk();
|
||||
mockFetchWithResolvedRoot('/new/root');
|
||||
|
||||
const changePromise = manager.handleSidecarStoragePathChange();
|
||||
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
|
||||
@@ -329,16 +403,93 @@ describe('SettingsManager sidecar storage', () => {
|
||||
body: JSON.stringify({ direction: 'relocate_root', force: true, old_root: '/old/root' }),
|
||||
}));
|
||||
expect(manager._loadedSidecarStoragePath).toBe('/new/root');
|
||||
expect(manager._loadedSidecarStorageRoot).toBe('/new/root');
|
||||
});
|
||||
|
||||
it('offers relocation from the default root when no custom path was set (#1142)', async () => {
|
||||
const manager = createManager();
|
||||
const { pathInput } = appendSidecarControls();
|
||||
const modal = appendMigrationModal();
|
||||
// No custom path: the raw setting is empty, but sidecars live in
|
||||
// the resolved default root.
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'centralized',
|
||||
sidecar_storage_path: '',
|
||||
sidecar_storage_root: '/settings/sidecars',
|
||||
};
|
||||
manager._loadedSidecarStoragePath = '';
|
||||
manager._loadedSidecarStorageRoot = '/settings/sidecars';
|
||||
pathInput.value = '/custom/dir';
|
||||
mockFetchWithResolvedRoot('/custom/dir');
|
||||
|
||||
const changePromise = manager.handleSidecarStoragePathChange();
|
||||
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
|
||||
modal.querySelector('[data-action="confirm-sidecar-migration"]').click();
|
||||
await changePromise;
|
||||
|
||||
expect(global.fetch).toHaveBeenCalledWith('/api/lm/sidecars/migrate', expect.objectContaining({
|
||||
body: JSON.stringify({ direction: 'relocate_root', force: true, old_root: '/settings/sidecars' }),
|
||||
}));
|
||||
});
|
||||
|
||||
it('offers relocation back to the default root when the path is cleared', async () => {
|
||||
const manager = createManager();
|
||||
const { pathInput } = appendSidecarControls();
|
||||
const modal = appendMigrationModal();
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'centralized',
|
||||
sidecar_storage_path: '/custom/dir',
|
||||
sidecar_storage_root: '/custom/dir',
|
||||
};
|
||||
manager._loadedSidecarStoragePath = '/custom/dir';
|
||||
manager._loadedSidecarStorageRoot = '/custom/dir';
|
||||
pathInput.value = '';
|
||||
mockFetchWithResolvedRoot('/settings/sidecars');
|
||||
|
||||
const changePromise = manager.handleSidecarStoragePathChange();
|
||||
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
|
||||
modal.querySelector('[data-action="confirm-sidecar-migration"]').click();
|
||||
await changePromise;
|
||||
|
||||
expect(global.fetch).toHaveBeenCalledWith('/api/lm/sidecars/migrate', expect.objectContaining({
|
||||
body: JSON.stringify({ direction: 'relocate_root', force: true, old_root: '/custom/dir' }),
|
||||
}));
|
||||
});
|
||||
|
||||
it('does not prompt when the resolved root is unchanged', async () => {
|
||||
const manager = createManager();
|
||||
const { pathInput } = appendSidecarControls();
|
||||
appendMigrationModal();
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'centralized',
|
||||
sidecar_storage_path: '/old/root',
|
||||
sidecar_storage_root: '/old/root',
|
||||
};
|
||||
manager._loadedSidecarStoragePath = '/old/root';
|
||||
manager._loadedSidecarStorageRoot = '/old/root';
|
||||
// Whitespace-only edit: saveInputSetting no-ops, root stays put.
|
||||
pathInput.value = '/old/root';
|
||||
mockFetchWithResolvedRoot('/old/root');
|
||||
|
||||
await manager.handleSidecarStoragePathChange();
|
||||
|
||||
const migrateCalls = global.fetch.mock.calls.filter(([url]) => url === '/api/lm/sidecars/migrate');
|
||||
expect(migrateCalls).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('does not prompt when the path changes in alongside mode', async () => {
|
||||
const manager = createManager();
|
||||
const { pathInput } = appendSidecarControls();
|
||||
appendMigrationModal();
|
||||
state.global.settings = { sidecar_storage_mode: 'alongside', sidecar_storage_path: '/old/root' };
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'alongside',
|
||||
sidecar_storage_path: '/old/root',
|
||||
sidecar_storage_root: '/old/root',
|
||||
};
|
||||
manager._loadedSidecarStoragePath = '/old/root';
|
||||
manager._loadedSidecarStorageRoot = '/old/root';
|
||||
pathInput.value = '/new/root';
|
||||
mockFetchOk();
|
||||
mockFetchWithResolvedRoot('/new/root');
|
||||
|
||||
await manager.handleSidecarStoragePathChange();
|
||||
|
||||
@@ -350,10 +501,15 @@ describe('SettingsManager sidecar storage', () => {
|
||||
const manager = createManager();
|
||||
const { pathInput } = appendSidecarControls();
|
||||
const modal = appendMigrationModal();
|
||||
state.global.settings = { sidecar_storage_mode: 'centralized', sidecar_storage_path: '/old/root' };
|
||||
state.global.settings = {
|
||||
sidecar_storage_mode: 'centralized',
|
||||
sidecar_storage_path: '/old/root',
|
||||
sidecar_storage_root: '/old/root',
|
||||
};
|
||||
manager._loadedSidecarStoragePath = '/old/root';
|
||||
manager._loadedSidecarStorageRoot = '/old/root';
|
||||
pathInput.value = '/new/root';
|
||||
mockFetchOk();
|
||||
mockFetchWithResolvedRoot('/new/root');
|
||||
|
||||
const changePromise = manager.handleSidecarStoragePathChange();
|
||||
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
|
||||
|
||||
@@ -619,3 +619,112 @@ async def test_migrate_covers_excluded_models(
|
||||
# The excluded model is not in the cache, so cache reconciliation is a
|
||||
# no-op for it and only the cached entry gets persisted.
|
||||
assert use_case._test_scanner.persist_calls == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_reports_destination_path_too_long(
|
||||
library_root: Path, sidecar_root: Path, monkeypatch: pytest.MonkeyPatch
|
||||
):
|
||||
"""Windows WinError 206 becomes an actionable per-model error (#1142)."""
|
||||
|
||||
_set_mode("centralized")
|
||||
model = _write_model(library_root, "model")
|
||||
sidecar = _write_sidecar(library_root, "model", model, preview_ext=None)
|
||||
|
||||
use_case = _make_use_case([str(model)])
|
||||
|
||||
def failing_move(src: str, dst: str) -> None:
|
||||
exc = OSError("The filename or extension is too long")
|
||||
exc.winerror = 206 # ERROR_FILENAME_EXCED_RANGE
|
||||
raise exc
|
||||
|
||||
monkeypatch.setattr(use_case, "_move_file", failing_move)
|
||||
summary = await use_case.migrate_to_centralized(force=True)
|
||||
|
||||
assert summary["success"] is False
|
||||
assert summary["error_count"] == 1
|
||||
assert summary["moved"] == 0
|
||||
message = summary["errors"][0]["error"]
|
||||
assert "too long" in message
|
||||
assert "shallower sidecar storage directory" in message
|
||||
# Failed move leaves the source untouched: no partial migration.
|
||||
assert sidecar.exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_reports_enametoolong(
|
||||
library_root: Path, sidecar_root: Path, monkeypatch: pytest.MonkeyPatch
|
||||
):
|
||||
"""ENAMETOOLONG gets the same actionable message as WinError 206."""
|
||||
|
||||
import errno as errno_module
|
||||
|
||||
_set_mode("centralized")
|
||||
model = _write_model(library_root, "model")
|
||||
_write_sidecar(library_root, "model", model, preview_ext=None)
|
||||
|
||||
use_case = _make_use_case([str(model)])
|
||||
|
||||
def failing_move(src: str, dst: str) -> None:
|
||||
raise OSError(errno_module.ENAMETOOLONG, "File name too long")
|
||||
|
||||
monkeypatch.setattr(use_case, "_move_file", failing_move)
|
||||
summary = await use_case.migrate_to_centralized(force=True)
|
||||
|
||||
assert summary["success"] is False
|
||||
assert "too long" in summary["errors"][0]["error"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_keeps_unrelated_oserror_message(
|
||||
library_root: Path, sidecar_root: Path, monkeypatch: pytest.MonkeyPatch
|
||||
):
|
||||
"""Non-path-length OSErrors propagate with their original message."""
|
||||
|
||||
import errno as errno_module
|
||||
|
||||
_set_mode("centralized")
|
||||
model = _write_model(library_root, "model")
|
||||
_write_sidecar(library_root, "model", model, preview_ext=None)
|
||||
|
||||
use_case = _make_use_case([str(model)])
|
||||
|
||||
def failing_move(src: str, dst: str) -> None:
|
||||
raise OSError(errno_module.EACCES, "Permission denied")
|
||||
|
||||
monkeypatch.setattr(use_case, "_move_file", failing_move)
|
||||
summary = await use_case.migrate_to_centralized(force=True)
|
||||
|
||||
assert summary["success"] is False
|
||||
message = summary["errors"][0]["error"]
|
||||
assert "Permission denied" in message
|
||||
assert "too long" not in message
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_root_reports_destination_path_too_long(
|
||||
library_root: Path, sidecar_root: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
):
|
||||
"""Root relocation surfaces path-length failures per file (#1142)."""
|
||||
|
||||
_set_mode("centralized")
|
||||
old_root = tmp_path / "old_sidecars"
|
||||
old_mirror = old_root / "component" / "sub"
|
||||
old_mirror.mkdir(parents=True)
|
||||
(old_mirror / "model.metadata.json").write_text("{}", encoding="utf-8")
|
||||
|
||||
use_case = _make_use_case([])
|
||||
|
||||
def failing_move(src: str, dst: str) -> None:
|
||||
exc = OSError("The filename or extension is too long")
|
||||
exc.winerror = 206
|
||||
raise exc
|
||||
|
||||
monkeypatch.setattr(use_case, "_move_file", failing_move)
|
||||
summary = await use_case.migrate_root(str(old_root), force=True)
|
||||
|
||||
assert summary["success"] is False
|
||||
assert summary["moved"] == 0
|
||||
assert any("too long" in entry["error"] for entry in summary["errors"])
|
||||
# Source file stays in the old tree.
|
||||
assert (old_mirror / "model.metadata.json").exists()
|
||||
|
||||
Reference in New Issue
Block a user