From 0de006f47d3286bdeca8ae38d85731ff4a0c7c8a Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Wed, 16 Sep 2026 21:13:31 +0200 Subject: [PATCH] fix: let the flow chat take a typed model id and keep a shared thinking input editable Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/ChatModelSettings.svelte | 99 +++++++++++-------- .../copilot/ReasoningEffortSlider.svelte | 4 +- .../copilot/chatModelSettings.test.ts | 3 +- .../components/copilot/chatModelSettings.ts | 5 + .../conversations/FlowChatInterface.svelte | 6 +- .../FlowChatModelSettings.svelte | 67 ++++++++----- .../conversations/agentChatInputs.test.ts | 23 ++++- .../flows/conversations/agentChatInputs.ts | 32 +++++- 8 files changed, 159 insertions(+), 80 deletions(-) diff --git a/frontend/src/lib/components/copilot/ChatModelSettings.svelte b/frontend/src/lib/components/copilot/ChatModelSettings.svelte index 2cbdc38d61..a1e677d46b 100644 --- a/frontend/src/lib/components/copilot/ChatModelSettings.svelte +++ b/frontend/src/lib/components/copilot/ChatModelSettings.svelte @@ -48,8 +48,8 @@ }) : undefined ) - // The stops, the one in use and the trigger's suffix are decided together, in one - // tested place: they have to agree, and three rounds of review found them disagreeing. + // The stops, the one in use and the trigger's suffix are decided together, in one tested + // place: a stop the slider shows as `off` must not read as the provider's `none` on the button. const display = $derived(reasoningDisplay(reasoning, capability, effective)) const stops = $derived(display.stops) const currentStop = $derived(display.currentStop) @@ -117,7 +117,49 @@ {/snippet} -{#snippet section(sec: ChoiceSection, item: MeltItem)} +{#snippet typedField( + value: string, + placeholder: string, + onCommit: (value: string) => void, + close: () => void +)} + {#key value} + onCommit(e.currentTarget.value.trim()), + // Capture, not bubble: Svelte delegates `keydown` to the root, which sits above the + // menu — so a bubble handler here would run only after melt's own listener had read + // the key as typeahead and moved focus. A capture key is not delegatable, so this + // becomes a real listener on the input and sees the event first. + onkeydowncapture: (e) => { + // Escape cancels: let it reach the menu with the value untouched. + if (e.key === 'Escape') return + // Tab closes the menu, unmounting this field before focus moves, so no change + // event would ever fire. Commit on the way past. + if (e.key === 'Tab') { + onCommit(e.currentTarget.value.trim()) + return + } + // Enter means done: commit and close, rather than leaving the menu open around a + // field the commit is about to rebuild. + if (e.key === 'Enter') { + e.preventDefault() + onCommit(e.currentTarget.value.trim()) + close() + return + } + // Everything else is typing; the menu reads loose keys as typeahead. + e.stopPropagation() + } + }} + /> + {/key} +{/snippet} + +{#snippet section(sec: ChoiceSection, item: MeltItem, close: () => void)}
{sec.label}
{#if sec.loading}
@@ -140,6 +182,19 @@ {/each}
{/if} + {#if sec.custom && !sec.loading} + {@const custom = sec.custom} +
+ {@render typedField( + '', + custom.placeholder, + (value) => { + if (value) custom.onCommit(value) + }, + close + )} +
+ {/if} {/snippet} {#snippet rows(items: Item[], item: MeltItem, builders: MeltBuilders)} @@ -188,7 +243,7 @@ {/if} {#each config.sections ?? [] as sec (sec.label)}
- {@render section(sec, item)} + {@render section(sec, item, close)}
{/each} {#if reasoning} @@ -213,41 +268,7 @@ would be a guess, and offering nothing would leave it settable nowhere. -->
Thinking
- {#key reasoning.value} - reasoning?.onSelect(e.currentTarget.value.trim()), - // Capture, not bubble: Svelte delegates `keydown` to the root, which sits - // above the menu — so a bubble handler here would run only after melt's own - // listener had read the key as typeahead and moved focus. A capture key is - // not delegatable, so this becomes a real listener on the input and sees the - // event first. - onkeydowncapture: (e) => { - // Escape cancels: let it reach the menu with the value untouched. - if (e.key === 'Escape') return - // Tab closes the menu, unmounting this field before focus moves, so no - // change event would ever fire. Commit on the way past. - if (e.key === 'Tab') { - reasoning?.onSelect(e.currentTarget.value.trim()) - return - } - // Enter means done: commit and close, rather than leaving the menu open - // around a field the commit is about to rebuild. - if (e.key === 'Enter') { - e.preventDefault() - reasoning?.onSelect(e.currentTarget.value.trim()) - close() - return - } - // Everything else is typing; the menu reads loose keys as typeahead. - e.stopPropagation() - } - }} - /> - {/key} + {@render typedField(reasoning.value ?? '', 'none', reasoning.onSelect, close)}
Windmill has no thinking levels for this provider — type what it accepts.
diff --git a/frontend/src/lib/components/copilot/ReasoningEffortSlider.svelte b/frontend/src/lib/components/copilot/ReasoningEffortSlider.svelte index fb184bf9c4..24ea4cf8aa 100644 --- a/frontend/src/lib/components/copilot/ReasoningEffortSlider.svelte +++ b/frontend/src/lib/components/copilot/ReasoningEffortSlider.svelte @@ -31,8 +31,8 @@ /** * A `current` naming no stop is a real state, not a missing one: an agent that leaves the - * effort unset sends nothing and the provider decides. Three things follow, and each has - * been got wrong on its own — keep them together. + * effort unset sends nothing and the provider decides. Three things follow, and they only + * hold together. * * The thumb rests at the start, because a range input always has one somewhere, and * `overrideLabel` is what tells the reader this is not the lowest stop. The track is diff --git a/frontend/src/lib/components/copilot/chatModelSettings.test.ts b/frontend/src/lib/components/copilot/chatModelSettings.test.ts index b1dda67808..04d1834556 100644 --- a/frontend/src/lib/components/copilot/chatModelSettings.test.ts +++ b/frontend/src/lib/components/copilot/chatModelSettings.test.ts @@ -129,8 +129,7 @@ describe('reasoningDisplay', () => { describe('carriedReasoning', () => { const cap = (model: string) => getReasoningCapability('openai', model) - // The bug this exists for: picking a model that cannot think left the old level in the - // flow input, and the run sent it anyway. + // A level carried onto a model that cannot think stays in the flow input, and the run sends it. it('drops a level the new model does not have', () => { expect(carriedReasoning('high', '', cap('gpt-4o'))).toBeUndefined() expect(carriedReasoning('xhigh', '', cap('gpt-5.1'))).toBeUndefined() diff --git a/frontend/src/lib/components/copilot/chatModelSettings.ts b/frontend/src/lib/components/copilot/chatModelSettings.ts index e07fc11f6c..3fce0270d1 100644 --- a/frontend/src/lib/components/copilot/chatModelSettings.ts +++ b/frontend/src/lib/components/copilot/chatModelSettings.ts @@ -31,6 +31,11 @@ export type ChoiceSection = { /** Shown when the list is empty and settled. */ emptyMessage?: string maxHeight?: string + /** + * A typed entry under the options, for a value the list does not hold: an endpoint with no + * listing, or a model newer than the catalogue. An empty entry commits nothing. + */ + custom?: { placeholder: string; onCommit: (value: string) => void } } export type ChatModelSettingsConfig = { diff --git a/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte b/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte index 199d9f9416..f6aa0670c4 100644 --- a/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte +++ b/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte @@ -17,6 +17,7 @@ agentModelGap, composerOwnedInputs, resolveAgentModelWiring, + showsModelButton, withoutRejectedEffort } from './agentChatInputs' @@ -61,6 +62,7 @@ // An agent with nothing to call cannot answer, and the composer cannot fix it, so the // chat says what to go and do instead of offering controls that write nowhere. const modelGap = $derived(agentModelGap(modelWiring)) + const showModelButton = $derived(showsModelButton(modelWiring)) // LocalStorage helpers const STORAGE_KEY_PREFIX = 'windmill_flow_chat_inputs_' @@ -249,7 +251,7 @@ {/if}
{/if} - {#if modelWiring} + {#if modelWiring && showModelButton} setFields({ model: m, ...effortPatch(m) }) })), loading: models.loading, - emptyMessage: provider ? 'No model available' : 'Pick a provider first' + emptyMessage: provider ? 'No model listed' : 'Pick a provider first', + // The step's own provider picker takes any model id, and the modal does not + // ask for this input: without a typed entry, an endpoint that lists nothing + // leaves the run with no model. + custom: provider + ? { + placeholder: 'Custom model id', + onCommit: (m) => setFields({ model: m, ...effortPatch(m) }) + } + : undefined } ] : undefined, - // Always present, whatever we can say about it: the run uses an effort either way, and - // the control is the only place it can be read or set. What varies is the state it - // renders — a ladder, a typed token, why there is none, or what the flow fixed. - reasoning: { - provider, - model: typeof model === 'string' && model ? model : undefined, - value: typeof effort === 'string' ? effort : undefined, - // An agent writes the provider-native token straight into its step, so there is no - // sentinel to translate later. Where a model disables by omission instead, the empty - // string is that off: the run reads an empty `reasoning_effort` as absent - // (types.rs `get_reasoning_effort`). - offToken: - provider && typeof model === 'string' && model - ? (explicitOffToken(provider, model) ?? '') - : '', - // An agent step omits `reasoning_effort` when it is unset, so the provider picks — - // naming a level would claim something the run does not do. - sendsDefaultWhenUnset: false, - writable: effortEditable, - typedWhenUnknown: true, - onSelect: (token) => setFields({ reasoning_effort: token }) - } + // Present whatever we can say about it, since the run uses an effort either way: a ladder, + // a typed token, why there is none, or what the flow fixed. Absent only when the modal + // is the effort's editor. + reasoning: effortInModal + ? undefined + : { + provider, + model: typeof model === 'string' && model ? model : undefined, + value: typeof effort === 'string' ? effort : undefined, + // An agent writes the provider-native token straight into its step, so there is no + // sentinel to translate later. Where a model disables by omission instead, the empty + // string is that off: the run reads an empty `reasoning_effort` as absent + // (types.rs `get_reasoning_effort`). + offToken: + provider && typeof model === 'string' && model + ? (explicitOffToken(provider, model) ?? '') + : '', + // An agent step omits `reasoning_effort` when it is unset, so the provider picks — + // naming a level would claim something the run does not do. + sendsDefaultWhenUnset: false, + writable: effortEditable, + typedWhenUnknown: true, + onSelect: (token) => setFields({ reasoning_effort: token }) + } }) diff --git a/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts b/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts index 84ea1648ab..0c4fdbac8a 100644 --- a/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts +++ b/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts @@ -5,6 +5,7 @@ import { composerOwnedInputs, parseProviderTransform, resolveAgentModelWiring, + showsModelButton, withoutRejectedEffort } from './agentChatInputs' import type { FlowModule } from '$lib/gen' @@ -113,9 +114,9 @@ describe('agentModelWiringInputs', () => { expect(agentModelWiringInputs(wiring)?.sort()).toEqual(['k', 'm', 'r']) }) - // The button always draws a thinking control — a ladder, a typed token, or why there is - // neither — so a wired effort is the button's whatever the registry knows about the model. - it('always claims a wired reasoning_effort, whatever the provider', () => { + // A thinking control is usable whatever the registry knows about the model — a ladder, a + // typed token, or why there is neither — so a wired effort is the button's there. + it('claims a wired reasoning_effort whatever the provider', () => { const custom = resolveAgentModelWiring([ agent( `({ "kind": "customai", "resource": "$res:u/admin/custom", model: flow_input.m, reasoning_effort: flow_input.thinking })` @@ -130,10 +131,22 @@ describe('agentModelWiringInputs', () => { ]) expect(agentModelWiringInputs(known)).toEqual(['thinking']) }) + + // Agents on different fixed models leave the button no model to place the effort on, so + // it would say "Pick a model first" with nothing to pick. The modal keeps the input. + it('leaves a shared effort to the modal when the agents fix different models', () => { + const resource = `"kind": "anthropic", "resource": "$res:u/admin/claude"` + const wiring = resolveAgentModelWiring([ + agent(`({ ${resource}, "model": "claude-sonnet-5", reasoning_effort: flow_input.thinking })`), + agent(`({ ${resource}, "model": "claude-opus-5", reasoning_effort: flow_input.thinking })`) + ]) + expect(wiring?.fields.reasoning_effort).toBe('thinking') + expect(agentModelWiringInputs(wiring)).toEqual([]) + expect(showsModelButton(wiring)).toBe(false) + }) }) -// The modal is whatever this does not return, so the two can no longer disagree about an -// input — and what a control can do *right now* is deliberately not part of the answer. +// The modal is whatever this does not return, so the two cannot disagree about an input. describe('composerOwnedInputs', () => { const wiring = () => resolveAgentModelWiring([ diff --git a/frontend/src/lib/components/flows/conversations/agentChatInputs.ts b/frontend/src/lib/components/flows/conversations/agentChatInputs.ts index 0290f449be..472929649d 100644 --- a/frontend/src/lib/components/flows/conversations/agentChatInputs.ts +++ b/frontend/src/lib/components/flows/conversations/agentChatInputs.ts @@ -322,19 +322,41 @@ export function agentModelGap(wiring: AgentModelWiring | undefined): string | un * resource leaves the button nothing to write it with, and hiding it would leave the run * without a provider kind and no way to supply one. * - * `reasoning_effort` needs no such condition: the button always draws a thinking control, - * whatever it can say about the model — a ladder, a typed token, or why there is neither — so - * a wired effort is always the button's. Asking for it in the modal as well would be a second - * editor for a field that already has one. + * `reasoning_effort` is the button's only where it has a model to place the effort against + * (see `composerDrivesEffort`). */ export function agentModelWiringInputs(wiring: AgentModelWiring | undefined): string[] { if (!wiring) return [] if (wiring.whole) return [wiring.whole] - const driven: ProviderField[] = ['resource', 'model', 'reasoning_effort'] + const driven: ProviderField[] = ['resource', 'model'] if (wiring.fields.resource !== undefined) driven.push('kind') + if (composerDrivesEffort(wiring)) driven.push('reasoning_effort') return driven.map((field) => wiring.fields[field]).filter((name): name is string => !!name) } +/** + * Whether the model button draws the thinking control for a wired effort. + * + * Every thinking state but one is usable: a ladder, a typed token, or why the model has + * neither. The exception is a model the button cannot name, as when agents share an effort + * input but fix different models. It would say "Pick a model first" with no model to pick, + * so the effort stays with the modal. + */ +export function composerDrivesEffort(wiring: AgentModelWiring): boolean { + if (wiring.whole) return true + if (wiring.fields.reasoning_effort === undefined) return false + return wiring.fields.model !== undefined || !agentFieldEmpty(wiring, 'model') +} + +/** + * Whether the composer draws a model button at all: something to write, or a fixed model to + * name. Agents that fix different models leave neither. + */ +export function showsModelButton(wiring: AgentModelWiring | undefined): boolean { + if (!wiring) return false + return agentModelWiringInputs(wiring).length > 0 || !agentFieldEmpty(wiring, 'model') +} + /** * The flow inputs the composer edits, and therefore the ones the Configure-inputs modal must * not ask for. The modal is whatever is left, so this is the single answer to "who edits