Repository navigation
feat(gui3): remove backend preset rail, move assignment to global Presets view (#2432) - #2433
Conversation
…sets view (#2432) Backend view no longer behaves like a model: the preset rail, visual preset picker, and model-like cell-selection affordance are removed. A backend has no preset by default; an assigned backend preset is now shown read-only on its cell only when present. Backend-preset assignment moves to the global Presets slideover, mirroring the model-assignment flow. The new 'Apply to a backend' control communicates that the binding is GLOBAL ('applies globally to all models using this backend') and that backend presets are optional runtime defaults that MERGE with model presets rather than replace them. A new 'Applied to backends' zone lists global bindings with a Detach action. Backend-preset binding stays client-local (localStorage, invariant #11). Tests: removed obsolete A51-A54/A58 (cell-select + rail notice), added A167-A173 covering rail/picker removal, axe scan, accessible backend-assignment control with global copy, keyboard operability + role=status announcement, and read-only display. Full suite: 183 passed, 7 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks — the UI direction matches #2432 well: Backend view is de-modelified, assignment moved into Presets, and the a11y coverage is targeted.
I would not merge this quite yet; I found two functional gaps:
-
Backend preset assignments look inert at runtime. The PR stores backend_presets and displays the binding, but the load path still appears to only use the model preset. api.loadModel() builds stagedOptions from recipeOptionsForModel(modelName, cachedModelInfo), and recipeOptionsForModel() only reads activePresetForModel(modelName); it does not read/merge activePresetForBackend(...) or loadBackendApplied(). That means the new backend binding can be visible in the UI but never affect effective recipe_options, which misses the #2432 “merged result” acceptance criterion.
-
Default can be stored as a real backend binding. The Backend view treats DEFAULT_PRESET.id as “no assigned preset” and hides it, but the new Presets flow allows assigning Default because it is compatible with all backends. That creates an “Applied to backends” row for Default while the Backend view intentionally shows no chip. I’d make assigning Default detach/no-op, or disable backend assignment for Default with copy that “no backend preset” is the default state.
|
@fl0rianr thanks for the review — UI direction confirmed, and you're right on both functional gaps. Plan: GAP 1 (inert backend bindings): Wire GAP 2 (Default as a backend binding): Disable backend assignment for the Default preset with accessible copy (Default is the New a11y tests A174+ for both. Will push and ping you for re-review. — Mattingly (Squad) |
… assign (#2432, PR #2433 review) Addresses fl0rianr's CHANGES_REQUESTED review on PR #2433. GAP 1 — backend-preset bindings were inert on load. recipeOptionsForModel() only read the model preset, so an assigned backend preset never affected the effective recipe_options. Added activePresetForModelBackend() (matches a backend binding whose recipe-part equals one of the model's recipes; Default = no backend preset) and merged its args UNDER the model preset so model-specific values win on conflict (backend = global base defaults). Mirrors main's more-specific-source-wins arg merge. GAP 2 — assigning the Default preset to a backend stored a real binding, creating an 'Applied to backends' row with no matching Backend-view chip. Backend assignment is now disabled for the Default preset in the slideover with accessible copy (programmatically associated via aria-describedby), and handleApplyBackend() defensively detaches/no-ops if Default is ever passed. Tests: new A174 (merge asserted on /load body), A175 (Default disabled + a11y copy/describedby), A176 (axe scan of disabled state). 186 passed, 7 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@fl0rianr both gaps are fixed and pushed (5830b25). Ready for re-review. GAP 1 — backend presets now affect the load path
GAP 2 — Default can no longer be stored as a backend binding
Tests: added A174 (asserts the merged — Mattingly (Squad) |
|
Thanks for addressing the previous two review points. The Default-preset handling is much cleaner now, and the runtime merge path is no longer inert. I still think this needs one more fix before merge:
The better path is to keep this backend-level, as the UI copy and #2432 acceptance criteria already define it. Please resolve the actual backend key before applying the backend preset, then merge only the preset for that exact The new A174 test currently locks in the over-broad behavior: it binds a backend preset to Also, in live testing I was able to attach presets to unsupported backends. That should not be allowed. The Presets view should filter or disable unsupported backend options based on Once the resolver uses the concrete backend key and unsupported backends are blocked in the assignment UI, this should be good to merge. |
|
@fl0rianr Thanks for the third review — acknowledged. Plan for the three remaining items:
Will push and request re-review. — Mattingly (Squad) |
…rted backends (#2432) PR #2433 round-3 review (fl0rianr). Three items: 1. activePresetForModelBackend() now resolves the CONCRETE backend a load will use (explicit load options -> model-preset recipe_options -> recipe default_backend from /system-info) and merges ONLY the preset bound to that exact recipe:backend key. Previously it matched recipe-level and applied a cpu-bound preset to every llamacpp load (vulkan/cuda/metal included). recipeOptionsForModel()/api.loadModel() thread explicit options + system-info. 2. Tests: A174 now proves EXACT backend matching (model resolves to concrete cpu via recipe default_backend); A177 negative test proves a llamacpp:cpu preset does NOT merge into a llamacpp:vulkan load. 3. Presets UI gates assignment on /system-info backend state: an 'unsupported' backend option is disabled with screen-reader copy + title and is not assignable (defense-in-depth in handleApplyBackend). A178/A179 cover it. 189 passed, 7 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@fl0rianr Round-3 fixes pushed (commit Item 1 — exact backend resolution (
The backend preset's own value is deliberately not consulted (would be circular). Item 2 — tests (
Item 3 — block unsupported backends (
Suite: 189 passed, 7 skipped (was 186/7; +3). New a11y max = A179. Ready for re-review — thanks! — Mattingly (Squad) |
…sets view (#2432) (#2433) * feat(gui3): remove backend preset rail; move assignment to global Presets view (#2432) Backend view no longer behaves like a model: the preset rail, visual preset picker, and model-like cell-selection affordance are removed. A backend has no preset by default; an assigned backend preset is now shown read-only on its cell only when present. Backend-preset assignment moves to the global Presets slideover, mirroring the model-assignment flow. The new 'Apply to a backend' control communicates that the binding is GLOBAL ('applies globally to all models using this backend') and that backend presets are optional runtime defaults that MERGE with model presets rather than replace them. A new 'Applied to backends' zone lists global bindings with a Detach action. Backend-preset binding stays client-local (localStorage, invariant #11). Tests: removed obsolete A51-A54/A58 (cell-select + rail notice), added A167-A173 covering rail/picker removal, axe scan, accessible backend-assignment control with global copy, keyboard operability + role=status announcement, and read-only display. Full suite: 183 passed, 7 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(gui3): make backend presets affect load + disable Default backend assign (#2432, PR #2433 review) Addresses fl0rianr's CHANGES_REQUESTED review on PR #2433. GAP 1 — backend-preset bindings were inert on load. recipeOptionsForModel() only read the model preset, so an assigned backend preset never affected the effective recipe_options. Added activePresetForModelBackend() (matches a backend binding whose recipe-part equals one of the model's recipes; Default = no backend preset) and merged its args UNDER the model preset so model-specific values win on conflict (backend = global base defaults). Mirrors main's more-specific-source-wins arg merge. GAP 2 — assigning the Default preset to a backend stored a real binding, creating an 'Applied to backends' row with no matching Backend-view chip. Backend assignment is now disabled for the Default preset in the slideover with accessible copy (programmatically associated via aria-describedby), and handleApplyBackend() defensively detaches/no-ops if Default is ever passed. Tests: new A174 (merge asserted on /load body), A175 (Default disabled + a11y copy/describedby), A176 (axe scan of disabled state). 186 passed, 7 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(gui3): resolve concrete backend for preset binding + gate unsupported backends (#2432) PR #2433 round-3 review (fl0rianr). Three items: 1. activePresetForModelBackend() now resolves the CONCRETE backend a load will use (explicit load options -> model-preset recipe_options -> recipe default_backend from /system-info) and merges ONLY the preset bound to that exact recipe:backend key. Previously it matched recipe-level and applied a cpu-bound preset to every llamacpp load (vulkan/cuda/metal included). recipeOptionsForModel()/api.loadModel() thread explicit options + system-info. 2. Tests: A174 now proves EXACT backend matching (model resolves to concrete cpu via recipe default_backend); A177 negative test proves a llamacpp:cpu preset does NOT merge into a llamacpp:vulkan load. 3. Presets UI gates assignment on /system-info backend state: an 'unsupported' backend option is disabled with screen-reader copy + title and is not assignable (defense-in-depth in handleApplyBackend). A178/A179 cover it. 189 passed, 7 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Removes the preset rail and visual preset-selection flow from the Backend view, and moves backend-preset assignment into the global Presets view (#2432, milestone GUI3). A backend no longer behaves like a model: it has no preset by default, and an assigned backend preset is shown read-only on its cell only when present.
Backend presets are framed as optional global runtime-argument defaults that MERGE with (do not replace) model presets. The new assignment control in the Presets slideover clearly communicates the binding is global. Backend-preset binding stays client-local (localStorage, invariant #11) —
lemondis untouched.What changed
BackendManager.tsx— removed the preset rail, visual preset picker, and the model-likecell__select-btnselection affordance + all rail-only state. Cells now show an assigned preset read-only ([data-cell-preset]) only when one is actually assigned and compatible.PresetManager.tsx— added an "Apply to a backend" section in the slideover (mirrors the model flow) with explicit global copy ("This preset applies globally to all models using this backend"), an accessible<select>(aria-label + aria-describedby → global note), an "Assign globally" button, arole="status"polite success region, and an "Applied to backends" zone listing global bindings with a qualified Detach action. Backend options are derived from/system-info.styles.css— added.preset-backend-successstyling.tests/a11y.spec.ts— removed obsolete A51–A54/A58 (cell-select + rail notice region); added A167–A173.Acceptance criteria
Tests
npx playwright test --reporter=line→ 183 passed, 7 skipped (baseline 181 passed; removed 5 obsolete tests for removed features, added 7 new A167–A173).Notes
lemondchanges.Closes #2432
— Mattingly (Squad)