From 1cfb516d1bb5293c25f1dec2adf14d5f8d727bc7 Mon Sep 17 00:00:00 2001 From: shimmyshimmer Date: Sat, 25 Jul 2026 01:59:03 -0700 Subject: [PATCH] Tighten comments in the new sidebar and delete-guard code --- .../backend/hub/services/models/deletion.py | 28 +++++------ studio/backend/routes/settings.py | 10 ++-- .../tests/test_personalization_settings.py | 10 ++-- .../frontend/src/components/app-sidebar.tsx | 49 +++++++------------ .../components/model-selector/pickers.tsx | 27 ++++------ .../components/sidebar-nav-customizer.tsx | 15 ++---- .../stores/appearance-custom-store.ts | 13 ++--- .../features/settings/tabs/appearance-tab.tsx | 1 + 8 files changed, 57 insertions(+), 96 deletions(-) diff --git a/studio/backend/hub/services/models/deletion.py b/studio/backend/hub/services/models/deletion.py index 21e49eb5b8..efc5013b61 100644 --- a/studio/backend/hub/services/models/deletion.py +++ b/studio/backend/hub/services/models/deletion.py @@ -589,10 +589,9 @@ def _inference_backend_blocks_delete(repo_id: str) -> bool: def _diffusion_blocks_delete(repo_id: str) -> Optional[str]: """The 400 detail if the Images backend holds *repo_id*, else None. - Checks the ACTIVE engine (diffusers or native sd_cpp): on a native selection the diffusers - singleton reports unloaded while sd-cli still generates from the cached GGUF, so checking it - alone would let files be deleted mid-use. Same fail-open-on-acquire / - surface-on-query contract as :func:`_llama_cpp_blocks_delete`. + Queries the ACTIVE engine: on a native selection the diffusers singleton reports + unloaded while sd-cli still generates from the cached GGUF. Same + fail-open-on-acquire contract as :func:`_llama_cpp_blocks_delete`. """ try: from core.inference.diffusion_engine_router import get_active_diffusion_engine @@ -604,16 +603,13 @@ def _diffusion_blocks_delete(repo_id: str) -> Optional[str]: if status.get("loaded") and status.get("repo_id"): if _loaded_id_matches_repo(str(status["repo_id"]), repo_id): return "Unload the model before deleting" - # The native sd.cpp engine re-reads companion VAE / text-encoder files from the HF cache - # every generation, so deleting a companion repo (e.g. comfyanonymous/flux_text_encoders) - # while a native GGUF is loaded bricks the next generation. status().repo_id covers only - # the main GGUF, so also refuse the committed companion repos read from disk. + # sd.cpp re-reads companion VAE / text-encoder files every generation, and + # status().repo_id covers only the main GGUF, so refuse the companions too. for lid in getattr(engine, "loaded_repo_ids", tuple)(): if _loaded_id_matches_repo(str(lid), repo_id): return "Unload the model before deleting" - # Also refuse while a background image load is DOWNLOADING this repo (or its companion - # base): status().loaded is still False then, but deleting would remove blobs from under - # the in-flight download/assembly. + # A downloading repo still reports loaded=False, but deleting would pull blobs + # from under the in-flight fetch. for lid in getattr(engine, "loading_repo_ids", tuple)(): if _loaded_id_matches_repo(str(lid), repo_id): return "An Images model load is using this repo; wait for it to finish" @@ -621,11 +617,10 @@ def _diffusion_blocks_delete(repo_id: str) -> Optional[str]: def _video_blocks_delete(repo_id: str) -> Optional[str]: - """The 400 detail if the Video backend holds (or is downloading) *repo_id*, else None. + """The 400 detail if the Video backend holds or is fetching *repo_id*, else None. - Cached non-GGUF video repos surface in the Video picker with a delete action, so without - this a loaded/loading Wan / LTX / Hunyuan pipeline could lose its HF snapshot from under - it. Mirrors :func:`_diffusion_blocks_delete`. + Video repos share the On Device delete action, so a live Wan / LTX / Hunyuan + pipeline could otherwise lose its snapshot. Mirrors :func:`_diffusion_blocks_delete`. """ try: from core.inference.video import get_video_backend @@ -673,8 +668,7 @@ async def delete_cached_model_response( ): blocks_detail = "Unload the model before deleting" else: - # The chat guards above are chat-only; the Images / Video engines hold their own - # pipelines (and companion repos) whose GGUFs must not vanish from under them. + # The guards above are chat-only; Images / Video hold their own pipelines. blocks_detail = _diffusion_blocks_delete(repo_id) or _video_blocks_delete(repo_id) except Exception as e: logger.warning(f"Load-state verification failed for {repo_id}; refusing delete: {e}") diff --git a/studio/backend/routes/settings.py b/studio/backend/routes/settings.py index 796b0f5ac4..3c7f72e69a 100644 --- a/studio/backend/routes/settings.py +++ b/studio/backend/routes/settings.py @@ -735,8 +735,7 @@ SIDEBAR_MENU_ITEM_DEFAULTS = { } # Navigable sidebar rows the user can pin/reorder; the boolean is each id's -# default pin state, matching the shipped layout. Unpinned rows collect in the -# "More" flyout client-side; a single unpinned row is hidden there instead. +# default pin state, matching the shipped layout. SIDEBAR_NAV_ITEM_DEFAULTS = { "projects": True, "hub": True, @@ -840,8 +839,7 @@ class PersonalizationCustomization(BaseModel): default_factory = _default_sidebar_menu, max_length = MAX_SIDEBAR_MENU_INPUT_ITEMS, ) - # Order matters here: it is the sidebar's render order, so the validator - # preserves the client's sequence and only appends ids it didn't send. + # Order is the sidebar's render order, so the validator keeps the client's. sidebarNav: list[PersonalizationSidebarNavItem] = Field( default_factory = _default_sidebar_nav, max_length = MAX_SIDEBAR_NAV_INPUT_ITEMS, @@ -866,9 +864,7 @@ class PersonalizationCustomization(BaseModel): def _validate_sidebar_nav( cls, value: list[PersonalizationSidebarNavItem] ) -> list[PersonalizationSidebarNavItem]: - # Same contract as sidebarMenu: drop duplicate ids (keep the first) and - # re-append missing ones, so the stored list covers every nav row exactly - # once while keeping the client's order. + # Like sidebarMenu, but order is preserved: dedupe, then append missing. seen: set[str] = set() items = [item for item in value if not (item.id in seen or seen.add(item.id))] for item_id, pinned in SIDEBAR_NAV_ITEM_DEFAULTS.items(): diff --git a/studio/backend/tests/test_personalization_settings.py b/studio/backend/tests/test_personalization_settings.py index c9e5a2ee2d..3a5921ee69 100644 --- a/studio/backend/tests/test_personalization_settings.py +++ b/studio/backend/tests/test_personalization_settings.py @@ -151,8 +151,7 @@ def _sidebar_nav(items): def test_customization_sidebar_nav_defaults_match_shipped_layout(): - # An untouched payload pins the rows the sidebar ships with and leaves the - # rest for the "More" flyout, so a fresh account looks unchanged. + # A fresh account must look like the shipped sidebar. c = PersonalizationPayload().appearance.customization assert [(i.id, i.pinned) for i in c.sidebarNav] == [ ("projects", True), @@ -175,8 +174,7 @@ def test_customization_sidebar_nav_preserves_order_and_normalizes(): ] ) ) - # Order is the sidebar's render order, so the client's sequence survives: - # duplicates keep the first entry and unsent ids are appended with defaults. + # Client order survives; duplicates keep the first, unsent ids are appended. assert [(i.id, i.pinned) for i in p.appearance.customization.sidebarNav] == [ ("video", True), ("hub", False), @@ -443,8 +441,8 @@ def test_personalization_route_roundtrip_real_shape(monkeypatch): {"id": "chat", "visible": False}, {"id": "connections", "visible": False}, ], - # Reordered and partly unpinned, so the round-trip proves the - # sidebar's render order survives a save unchanged. + # Reordered and partly unpinned, so the round-trip proves order + # survives a save. "sidebarNav": [ {"id": "images", "pinned": True}, {"id": "video", "pinned": True}, diff --git a/studio/frontend/src/components/app-sidebar.tsx b/studio/frontend/src/components/app-sidebar.tsx index 449b138af3..6ff147ff71 100644 --- a/studio/frontend/src/components/app-sidebar.tsx +++ b/studio/frontend/src/components/app-sidebar.tsx @@ -205,8 +205,8 @@ const SETTINGS_TAB_MENU_ITEMS: Record< connections: { icon: CloudIcon, labelKey: "settings.tabs.connections" }, }; -// A navigable sidebar row. The same definition renders either as a top-level -// NavItem or as a MoreMenuItem, depending on the user's pin preference. +// One navigable row, rendered as a NavItem or a MoreMenuItem depending on its +// pin state. type NavRowDef = { icon: typeof ZapIcon; label: string; @@ -293,9 +293,7 @@ function preloadSilently(request: Promise): void { void request.catch(() => undefined); } -// Small "New" pill for recently shipped tabs. Same recipe as the brand "beta" -// badge (nav-badge font, --ui-font-scale sizing, nav token colours) so the two -// read as one design language. +// "New" pill for recent tabs. Same recipe as the brand "beta" badge. function NavBadge({ label, className }: { label: string; className?: string }) { return ( | null>(null); const openMore = useCallback(() => { @@ -689,8 +686,7 @@ export function AppSidebar() { const chatDisabled = trainingInProgress; - // One definition per navigable row, so the pinned list and the More flyout - // render the same row from the same source and can't drift apart. + // One definition per row, so pinned rows and the flyout can't drift apart. const navRows: Record = { projects: { icon: Folder01Icon, @@ -704,8 +700,7 @@ export function AppSidebar() { preloadSilently(router.preloadRoute({ to: "/projects" })); }, className: "group/projects-item relative", - // The inline "new project" affordance only makes sense on a real row; in - // the flyout the row is just a link. + // The inline "new project" affordance only fits a real row. children: (