feat(web): guided picker for subagent model + provider (Dashboard half of #67347) - #67557
DavidMetcalfe wants to merge 6 commits into
Conversation
Mirrors the Desktop implementation in NousResearch#67523 to close the Dashboard half of the UX gap reported in NousResearch#67347. The Dashboard's `ConfigPage` surfaced both fields as bare free-text inputs via the generic schema-driven `AutoField` renderer — same problem Desktop just fixed. Replaces the schema-driven rendering for `delegation.model` and `delegation.provider` with a paired provider + model picker sourced from `/api/model/options` (the same endpoint the chat model picker and MoA preset picker use). Key behaviors (matched to the Desktop implementation): - "Inherit from main agent" is a first-class dropdown option that writes '' to both keys — the documented default at hermes_cli/config.py:2297-2298 and the resolver at tools/delegate_tool.py:3056-3170. - Provider switch clears the model so the new provider never pairs with the old provider's model. - Custom endpoints with empty model catalogs fall back to a free-text input. - Out-of-catalog persisted models stay selectable. - Gateway-unreachable degrades to free-text inputs + warning banner, never locks the page. - Atomic write at the parent via two `setNestedValue` calls inside one `setConfig` — never persists a half-pair. Implementation notes: - Uses the Dashboard's existing `useState` + `useEffect` fetch pattern (no react-query setup in web/src/), wired to `api.getModelOptions()`. - i18n keys live under `t.config.*` (matching the Desktop's `t.settings.config.*` from NousResearch#67523) since these are config-editor affordances, not model-picker affordances. - All 14 non-English locales stubbed with English values — translators fill in real translations in a follow-up. Test gap (explicit, called out in PR description): the Dashboard has no React component test infrastructure (@testing-library/react, jsdom) and adding it for one component is scope creep. Cross-vendor review on the diff is the verification path; a follow-up test can land separately if reviewers want it. Fixes NousResearch#67347 (Dashboard half) Local verification: - pnpm tsc clean (apps/desktop equivalent: `tsc --noEmit -p web/tsconfig.app.json`) - All 89 existing web vitest tests still pass.
Two SHOULD-FIX items from cross-vendor review of the prior commit:
1. **Atomic state merge in ConfigPage.** Replaced two sequential
`setNestedValue` calls with a single functional `setConfig` update
that builds the merged object before commit. Eliminates the brief
window where one of the two keys could be updated in isolation.
2. **Loading state in the picker.** Added `isLoading` state, set
`disabled={isLoading}` on the provider <Select>, and rendered a
"Loading subagent catalog…" hint below while the gateway fetch is
in flight. `isLoading` toggles inside the existing `useEffect` via
`.finally()` so the cancelled-guard still applies.
Also adds `delegationLoading` to all 16 locale files (English +
15 non-English stubs; translators fill in real values in a follow-up).
Items intentionally not changed (with rationale recorded in the
Stage 2 brief): inline `key ===` check (matches Desktop impl),
`isDelegationModelPickerKey` helper (testable surface), hand-rolled
fetch (no react-query in web/src/), Input import (already correct).
Two NIT/SHOULD-FIX items from the second-round cross-vendor review: 1. **Race on unmount (NIT, GPT-OSS NousResearch#5).** Moved the explanatory comment that documents the `cancelled` gating pattern; the gate was already in place but now has the intent documented inline. 2. **A11y on loading hint (SHOULD-FIX, GPT-OSS NousResearch#7).** Added `aria-live="polite"` to the loading hint so screen readers announce when the catalog finishes loading.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for carrying the Dashboard half of #67347 and for preserving the existing model-options endpoint and profile-aware config flow.
Problems
web/src/components/DelegationModelProviderField.tsx:135drops a supported configuration mode:delegation.modelmay be set whiledelegation.providerremains empty. Current resolution deliberately preserves parent credentials in that state (tools/delegate_tool.py:3139-3149), and the documented contract calls it out atwebsite/docs/user-guide/configuration.md:2023. The new providerSelecthas no empty/provider-inherit option whenisInheritis false, so users cannot intentionally create or reliably edit this model-only override.
Suggested changes
- Preserve model-only/provider-inherit as a distinct picker state, then add a focused interaction test covering it alongside concrete-provider and full-inherit transitions.
Automated hermes-sweeper review.
Two cleanups caught by the post-merge eslint pass on the push: 1. **Inline `isDelegationModelPickerKey`.** The helper exported from the component file was used at exactly one call site (ConfigPage.tsx), exported to dodge the `react-refresh/only-export-components` rule. Inlining as `key === "delegation.provider"` is shorter, matches surrounding ConfigPage style, and removes the export entirely. 2. **Drop redundant `setIsLoading(true)`.** `useState(true)` already initializes the loading flag; the explicit `setIsLoading(true)` at the top of the effect was duplicating that and triggering the `react-hooks/set-state-in-effect` lint rule (intentionally a warning in the web codebase until patterns get refactored — see commit 02613a4). Removed with a comment explaining why.
teknium1
added
sweeper:risk-compatibility
Two real BLOCKERs from the sweeper + Stage 3 round 1 review on PR NousResearch#67557 (via the same review on NousResearch#67523): 1. **Circular deadlock**: the picker derived its active state from `draftProvider` + `draftModel` alone. Selecting MODEL_ONLY_VALUE from clean state ("", "") snapped the dropdown back to INHERIT_VALUE because the derivation maps blank+blank → inherit. 2. **Input unmount on backspace**: clearing the model field mid-edit unmounted the input because the derivation flipped back to inherit. The fix introduces explicit local state for the dropdown's selected value (`providerSelectValue`) — user INTENT — separately from the draft model/provider pair. The dropdown no longer depends on the model being non-empty to stay on MODEL_ONLY_VALUE; clearing the model keeps the user in the model-only branch with an empty input ready for typing. Provider-switch and profile-switch paths resync `providerSelectValue` from the persisted config via the existing `useEffect` with the echo-ref pattern (mirrors `FallbackModelsField` on Desktop). Also shortened the dropdown option label from "Custom model (use parent credentials)" (38 chars) to "Custom model" (12 chars); the full explanation moves to a helper line below the input.
|
Addressed the sweeper finding about
The resolver at Also shortened the dropdown option label from "Custom model (use parent credentials)" (38 chars) to "Custom model" — the full explanation now lives in the helper line below the input. Cross-vendor review (Flash + GPT-OSS in parallel) returned unanimous ACCEPTABLE on both substantive findings after the fix; the only flagged SHOULD-FIX items were already addressed in the implementation (verified by source-tracing). Diff: +189 / -31 across 18 files. Same approach should fix the equivalent gap on #67523 if the maintainer wants to apply it there too. |
…t OR Three cleanups caught during the post-merge walkthrough on the state-machine refactor: 1. **Extract `selectValueFor(provider, model)` helper.** The `(provider, model) → dropdown value` mapping was duplicated in the `useState` seed and the `useEffect` resync. Now both call sites use the same helper. 2. **Drop the defensive `isInherit` OR-clause.** Removed the `|| (draftProvider === "" && draftModel === "")` fallback from the `isInherit` derivation — the helper covers all three cases at construction, so the OR is dead code. 3. **Type the helper parameters explicitly.** `selectValueFor` is declared with `string` parameters, so TypeScript guarantees no `undefined` can reach the truthy checks (the GPT-OSS round-3 BLOCKER "could crash on undefined" is a TypeScript-narrowing misread — verified by source trace). tsc + vitest + eslint all clean: - tsc --noEmit -p web/tsconfig.app.json: 0 errors - vitest run: 89/89 passing - eslint src/components/DelegationModelProviderField.tsx: 0 warnings Diff: +16 / -23 (one file).
alt-glitch
added
type/feature
alt-glitch
removed
the
sweeper:risk-compatibility
|
@OutThisLife @teknium1 — Checking in on #67557. All required checks are passing, the PR is mergeable, and no concerns have been raised since the July 19 review. Status
Scope If everything looks good, would it be okay to merge — and #67523 as well, if you agree? |
|
@OutThisLife @teknium1 — Following up on my August 10 check-in with one piece of context that may be relevant here. Since then, #74375 (unified subagent model/reasoning controls) was closed without merging, while the related surfaces remain open: the CLI picker (#76480), the Desktop half (#67523), and this Dashboard half. If the direction for #67347 has shifted toward a single unified surface, I'm happy to reshape or close this PR to fit. Otherwise it's ready for review: all required checks are green on the current head, it's mergeable with no conflicts, and the single review finding (the model-only override state) was addressed in the July 19 comment above. If anything specific is blocking review — scope, the test-gap note in the description, or anything else — let me know and I'll address it. |
Assemble the existing guided picker and pin/config fallback matrix on NousResearch/hermes-agent main e9bccc9. Expose default delegation provider/model and ordered backups together on the Models screen, using the same scoped editor for the Advanced entry. Keep partial selections local, preserve model-only overrides, pin catalog and config operations to one connection/profile, and confirm persisted writes. Negotiate backend support so older runtimes cannot accept inert fallback settings through the new editor. Port the worker fallback decision into the post-decomposition runtime owner. Declared chains work with explicit pins; [] disables recovery; absent/null retains pin-aware defaults. Explicit inherit/parent is opt-in, canonical empty suppresses aliases, and returned route metadata is deeply child-owned. Reuse the canonical normalizer and existing recovery loop. Source implementation and provenance: - webtecnica: NousResearch#67523, source head fd6e822; omit its unrelated runner change. - Ayush Nangia: NousResearch#80479, source head a0a8d8d; preserve the pin/config matrix and spfcraze's model-only-pin correction. - Earlier fallback work: Ayush Nangia NousResearch#65052, Axl Ibiza NousResearch#80421, wz-heng NousResearch#80438, and Teknium NousResearch#80465 pin protection already on main. - devatnull NousResearch#81072 supplies explicit inheritance semantics, not its incompatible automatic inheritance under a pin. - TurgutKural NousResearch#101017 supplies alias compatibility, not its [] default. - Reports/design: DavidMetcalfe NousResearch#67347, mlahatte NousResearch#65038, and ScotterMonk NousResearch#94629. This is a co-authored current-main recomposition, not an unchanged cherry-pick of the historical commits. Original branches and review threads remain untouched. Dashboard NousResearch#67557 remains a separate surface. Fixes NousResearch#65038 Refs NousResearch#94629 Refs NousResearch#67347 Refs NousResearch#80450 Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com> Co-authored-by: Ayush Nangia <ayushnangia16@gmail.com>
3 participants
What does this PR do?
Closes the Dashboard half of the
delegation.model/delegation.providerpicker UX gap originally reported in #67347. Mirrors the Desktop
implementation in #67523 (same design, different React app).
The Dashboard's
ConfigPage(web/src/pages/ConfigPage.tsx) wassurfacting both fields as bare free-text inputs via the generic
schema-driven
AutoFieldrenderer — same UX problem Desktop just fixed.This PR replaces them with a paired provider + model picker sourced from
/api/model/options(the same endpoint the chat model picker and MoApreset picker already use).
Key behaviors (matched to the Desktop implementation):
writes
""to both keys — the documented default athermes_cli/config.py:2297-2298and the resolver attools/delegate_tool.py:3056-3170.the old provider's model.
modelslist (user-defined endpoints without probed models) fall back to a free-text input.
pick renders instead of blank.
a warning banner; the settings page is never locked.
The atomic two-key write happens inside
setConfigat the parent — onecombined update, never a half-pair.
Related Issue
Fixes #67347 (Dashboard half — Desktop half landed in #67523)
Type of Change
Changes Made
web/src/components/DelegationModelProviderField.tsx(new, +201 lines)— the picker. Uses the Dashboard's existing
useState+useEffectfetch pattern (no react-query in
web/src/), wired toapi.getModelOptions().web/src/pages/ConfigPage.tsx— hooks the picker intorenderFields.delegation.modelrow renders<DelegationModelProviderField>, thedelegation.providerrow returns null (avoiding a duplicate free-textinput below the picker). Both keys written atomically via two
setNestedValuecalls inside onesetConfig.web/src/i18n/en.ts— 7 new keys plus a function-typeddelegationCurrentlyInheriting: (model: string) => string(matchesthe Desktop i18n pattern).
web/src/i18n/types.ts— type counterparts.web/src/i18n/{ja,zh,zh-hant,af,de,es,fr,ga,hu,it,ko,pt,ru,tr,uk}.ts— 14 non-English locales stubbed with English values (all Dashboard
locales declare
Translationsdirectly; nodefineLocaleshortcutavailable).
How to Test
/config(or whatever the route is for theconfig editor — see
web/src/pages/ConfigPage.tsx).delegation.model. The row should render thenew picker (provider dropdown + model dropdown or free-text fallback)
instead of a free-text input.
default option and a helper line shows the inherited main model.
provider's catalog models (or show a free-text input if the catalog
is empty).
delegation.modelanddelegation.providerare written to the config (no half-pair persisted).
inputs + a warning banner instead of locking.
Checklist
Code
feat(...)etc.)pytest tests/ -qand all tests pass — N/A (this is a frontend change, no Python tests affected).Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/A (no new config keys).CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A.Test gap (explicit)
The Dashboard has no existing React component tests, no
@testing-library/react, and nojsdomconfigured invitest.config.ts.Adding test infra for one component is scope creep. Cross-vendor review
on the diff is included in the PR description; if reviewers want a
test, happy to add it as a follow-up (would require installing
@testing-library/react+jsdomand updatingvitest.config.ts).Screenshots / Logs
N/A — UI change, no logs to capture. The fix is verifiable by opening
the Dashboard's config page and observing the picker replace the
free-text input.
Notes
(
apps/desktop/src/app/settings/delegation-model-provider-field.tsx)but uses
useState/useEffectinstead ofuseQuerybecause theDashboard's
web/src/has no react-query setup.t.config.*(matching the Desktop'st.settings.config.*convention from feat(desktop): add guided subagent model/provider picker (#67347) #67523) since these areconfig-editor affordances, not model-picker affordances.
translations in a follow-up.