(MOT-4412) feat(llm-router): make the config form operator-friendly - #789
(MOT-4412) feat(llm-router): make the config form operator-friendly#789ytallo wants to merge 2 commits into
Conversation
Show live provider status, edit timeouts in minutes, and move the system-prompt override into a dialog so the cards stay scannable.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 58 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughThe LLM router configuration UI now receives the host, loads live provider status, renders provider cards, supports timeout conversion and routing heuristics, displays validation errors, and adds helper utilities with tests. The stylesheet updates the form’s controls, panels, prompts, and provider states. ChangesLLM router configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The redesigned configuration form currently has concrete correctness and availability risks: decimal timeout values cannot be entered reliably, existing timeout values may be displayed inaccurately, malformed probe patterns can freeze the browser, and provider refresh failures or overlapping requests can show misleading or stale status. These issues should be fixed before merge. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
llm-router/ui/src/configuration/index.tsx (1)
63-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLabel a live provider correctly when it is not in
providerIds.
allIdscomes fromproviderCardIds, which unions schema ids and configuredproviderskeys only. A provider that the router reports as live, but that has no schema entry and noproviders.<id>slice, is absent fromproviderIds. If such an id is the currentdefault_provideror a heuristic target, the picker labels it "(not connected)" even thoughlivecontains it.Check the live map before you add the fallback label.
🐛 Proposed fix
if (current && !providerIds.includes(current)) { + const liveName = names.get(current) options.push({ value: current, - label: `${providerDisplayName(current)} (not connected)`, + label: liveName ? providerDisplayName(current, liveName) : `${providerDisplayName(current)} (not connected)`, }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm-router/ui/src/configuration/index.tsx` around lines 63 - 76, Update providerOptions to check the live provider map before labeling a current or heuristic provider as “(not connected)”. When the fallback id is present in live, use providerDisplayName with its live display name; reserve the “not connected” suffix for ids absent from both providerIds and live.
🧹 Nitpick comments (1)
llm-router/ui/styles.css (1)
245-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExclude disabled buttons from the hover style.
The move buttons in
llm-router/ui/src/configuration/index.tsxuseclassName="llmr-cfg-remove"and setdisabledat the list boundaries. The hover rule does not exclude:disabled. A disabled ↑ or ↓ button therefore changes its colour and background on hover, which signals an action that cannot run.🎨 Proposed fix
-[data-iii-ui='llm-router'] .llmr-cfg-remove:hover, -[data-iii-ui='llm-router'] .llmr-cfg-add:hover, -[data-iii-ui='llm-router'] .llmr-cfg-toggle:hover { +[data-iii-ui='llm-router'] .llmr-cfg-remove:hover:not(:disabled), +[data-iii-ui='llm-router'] .llmr-cfg-add:hover:not(:disabled), +[data-iii-ui='llm-router'] .llmr-cfg-toggle:hover:not(:disabled) { color: var(--color-ink); background: var(--color-surface-active); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm-router/ui/styles.css` around lines 245 - 254, Update the llmr-cfg-remove hover selector so disabled buttons are excluded from the hover styling, while preserving hover behavior for enabled remove, add, and toggle controls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@llm-router/ui/src/configuration/heuristics.ts`:
- Around line 18-22: Update moveItem to return items unless both from and to are
integer indexes within the array bounds, before copying or calling splice;
preserve the no-op behavior when from equals to. Add boundary tests covering
negative, out-of-range, and non-integer source and destination indexes.
- Around line 3-13: Update winningHeuristicIndex so pattern matching cannot
block the browser main thread through catastrophic backtracking: replace
synchronous RegExp evaluation with a linear-time regex engine or delegate
evaluation to a terminable worker enforcing a strict timeout. Preserve the
existing null, row-skipping, and invalid-pattern behavior, and do not use
input-length limits as the sole mitigation.
In `@llm-router/ui/src/configuration/index.tsx`:
- Around line 359-376: Update the heuristic reorder controls in the
configuration component so keyboard focus follows the moved heuristic after
commit, rather than remaining on a reused index-keyed button or being lost when
the button becomes disabled. Use a stable row identity and ref/focus handling
around the moveItem/commit flow, preserving the existing boundary disabling and
ordering behavior.
- Around line 463-471: Update the cleanup callback to guard both `off()` and
`unreg?.()` within the same error-handling flow, ensuring `unreg?.()` is still
attempted when `off()` throws while preserving the existing disposed-SDK
handling.
- Around line 319-325: Update the probe echo block rendered when probe.trim() is
truthy: add aria-live="polite" to announce changes, and replace
heuristicRows[winner].provider with the corresponding providerDisplayName value
used elsewhere in the form while preserving the existing no-match message and
row numbering.
- Around line 186-192: Update the deep-link focus effect around props.focusField
to depend on the first field value rather than the focusField array identity,
while preserving the existing lookup and scroll behavior. Also change the focus
target from the non-focusable [data-field] wrapper to its inner control, or make
the wrapper programmatically focusable with tabIndex={-1}, so focus reliably
lands on the field.
- Around line 255-288: Update the SETTINGS_FIELDS input handling to maintain
per-field raw draft text locally, preserving intermediate decimal values such as
“1.” while focused. Convert valid input to milliseconds for commit, but only
derive the displayed text from stored values when the field is not focused;
retain existing values such as 100000 without unnecessary minute round-trip
rewriting.
- Around line 438-452: Update the refresh flow around refresh and its provider
state to track a separate load error, preserve the distinction between an empty
result and a failed router::provider::list call, and expose a concise retry hint
in the form. Race host.iii.trigger against a defined timeout so stalled requests
settle, and add a monotonic refresh sequence guard that ignores responses and
errors from older overlapping calls while retaining the existing unmount
cancellation behavior.
Apply the same fix in `@llm-router/ui/src/configuration/provider-cards.ts` around
lines 62 - 71: Covers the status mapping that cannot distinguish unavailable
runtime data from a successful empty list.
In `@llm-router/ui/src/configuration/provider-card.tsx`:
- Around line 198-201: Update the max_tokens handler in the provider card’s
onChange callback to accept a value only when n is a finite, non-negative
integer, preserving undefined for empty or invalid input. Replace the current
NaN-only validation with the uint64-compatible Number.isInteger(n) && n >= 0
check.
In `@llm-router/ui/styles.css`:
- Around line 101-119: Add a neutral `.llmr-cfg-status.is-unknown` variant
alongside the existing provider status rules, using the appropriate neutral
foreground and muted background design tokens. Keep the selector scoped to the
`llm-router` UI and preserve the existing status styles.
---
Outside diff comments:
In `@llm-router/ui/src/configuration/index.tsx`:
- Around line 63-76: Update providerOptions to check the live provider map
before labeling a current or heuristic provider as “(not connected)”. When the
fallback id is present in live, use providerDisplayName with its live display
name; reserve the “not connected” suffix for ids absent from both providerIds
and live.
---
Nitpick comments:
In `@llm-router/ui/styles.css`:
- Around line 245-254: Update the llmr-cfg-remove hover selector so disabled
buttons are excluded from the hover styling, while preserving hover behavior for
enabled remove, add, and toggle controls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 413e1868-7497-4c3e-9c17-6a22a7b3ef06
📒 Files selected for processing (13)
llm-router/ui/page.tsxllm-router/ui/src/configuration/duration.test.tsllm-router/ui/src/configuration/duration.tsllm-router/ui/src/configuration/field-error.tsxllm-router/ui/src/configuration/heuristics.test.tsllm-router/ui/src/configuration/heuristics.tsllm-router/ui/src/configuration/index.tsxllm-router/ui/src/configuration/pointers.test.tsllm-router/ui/src/configuration/pointers.tsllm-router/ui/src/configuration/provider-card.tsxllm-router/ui/src/configuration/provider-cards.test.tsllm-router/ui/src/configuration/provider-cards.tsllm-router/ui/styles.css
| export function winningHeuristicIndex(model: string, rows: { pattern: string; provider: string }[]): number | null { | ||
| const needle = model.trim() | ||
| if (!needle) return null | ||
| for (let i = 0; i < rows.length; i++) { | ||
| const pattern = rows[i].pattern | ||
| if (!pattern || !rows[i].provider) continue | ||
| try { | ||
| if (new RegExp(pattern).test(needle)) return i | ||
| } catch { | ||
| // An invalid operator regex never takes the router down. | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'winningHeuristicIndex|new RegExp\(|routing_heuristics|probe' llm-router/ui/srcRepository: iii-hq/workers
Length of output: 16524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- heuristics.ts ---'
cat -n llm-router/ui/src/configuration/heuristics.ts
printf '%s\n' '--- relevant configuration render and validation ---'
sed -n '110,180p' llm-router/ui/src/configuration/index.tsx
sed -n '290,410p' llm-router/ui/src/configuration/index.tsx
printf '%s\n' '--- tests ---'
cat -n llm-router/ui/src/configuration/heuristics.test.ts
printf '%s\n' '--- regex/config validation references ---'
rg -n -C 4 'routing_heuristics|pattern|RegExp|regex|regular expression' llm-router --glob '!**/*.lock'Repository: iii-hq/workers
Length of output: 36939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const pattern = '^(a+)+$'
const needle = 'a'.repeat(28) + '!'
const start = process.hrtime.bigint()
let result
try {
result = new RegExp(pattern).test(needle)
} catch (error) {
result = String(error)
}
const elapsedMs = Number(process.hrtime.bigint() - start) / 1e6
console.log(JSON.stringify({ pattern, needleLength: needle.length, result, elapsedMs }))
JSRepository: iii-hq/workers
Length of output: 232
Prevent catastrophic regex evaluation on the browser main thread.
winningHeuristicIndex synchronously evaluates patterns from the draft configuration against free-form probe input. A pattern such as ^(a+)+$ can block the UI for seconds. The catch block handles syntax errors only. Use a linear-time regex engine or a terminable worker with a strict timeout. Do not rely on input-length limits alone.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 9-9: Do not use variable for regular expressions
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
[warning] 9-9: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/heuristics.ts` around lines 3 - 13, Update
winningHeuristicIndex so pattern matching cannot block the browser main thread
through catastrophic backtracking: replace synchronous RegExp evaluation with a
linear-time regex engine or delegate evaluation to a terminable worker enforcing
a strict timeout. Preserve the existing null, row-skipping, and invalid-pattern
behavior, and do not use input-length limits as the sole mitigation.
Source: Linters/SAST tools
| export function moveItem<T>(items: T[], from: number, to: number): T[] { | ||
| if (to < 0 || to >= items.length || from === to) return items | ||
| const next = [...items] | ||
| const [row] = next.splice(from, 1) | ||
| next.splice(to, 0, row) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'moveItem\s*\(|from: number|to: number' llm-router/ui/srcRepository: iii-hq/workers
Length of output: 3373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- heuristics.ts ---'
cat -n llm-router/ui/src/configuration/heuristics.ts
printf '%s\n' '--- heuristics.test.ts ---'
cat -n llm-router/ui/src/configuration/heuristics.test.ts
printf '%s\n' '--- runtime splice probes ---'
node - <<'JS'
const cases = [
['negative from', ['a', 'b', 'c'], -1, 0],
['from >= length', ['a', 'b', 'c'], 3, 1],
['fractional from', ['a', 'b', 'c'], 1.5, 0],
['NaN from', ['a', 'b', 'c'], NaN, 1],
['negative fractional from', ['a', 'b', 'c'], -1.5, 0],
]
for (const [name, items, from, to] of cases) {
if (to < 0 || to >= items.length || from === to) {
console.log(name, JSON.stringify(items))
continue
}
const next = [...items]
const [row] = next.splice(from, 1)
next.splice(to, 0, row)
console.log(name, JSON.stringify(next))
}
JSRepository: iii-hq/workers
Length of output: 2808
Validate the source index before calling splice.
Invalid from values can move the wrong item or insert undefined. Return items unless both from and to are integer indexes within the array, and add boundary tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/heuristics.ts` around lines 18 - 22, Update
moveItem to return items unless both from and to are integer indexes within the
array bounds, before copying or calling splice; preserve the no-op behavior when
from equals to. Add boundary tests covering negative, out-of-range, and
non-integer source and destination indexes.
| useEffect(() => { | ||
| const field = props.focusField?.[0] | ||
| if (!field || !rootRef.current) return | ||
| const el = rootRef.current.querySelector<HTMLElement>( | ||
| `[data-field="${CSS.escape(field)}"]`, | ||
| ) | ||
| const el = rootRef.current.querySelector<HTMLElement>(`[data-field="${CSS.escape(field)}"]`) | ||
| el?.scrollIntoView({ block: 'center' }) | ||
| el?.focus() | ||
| }, [props.focusField]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the deep-link focus effect against unstable focusField identity.
The effect depends on props.focusField. The host passes a readonly string[]. If the host recreates that array on each render, this effect re-runs and calls scrollIntoView plus focus again. The view then jumps while the operator edits a field. Depend on the first entry instead, which is the only value the effect reads.
Also note that [data-field] wrappers are div elements, for example line 197. HTMLElement.focus() on a div without tabindex does nothing. Target the inner control, or add tabIndex={-1} to the wrapper, if you want focus to land.
🐛 Proposed fix for the dependency identity
const rootRef = useRef<HTMLDivElement>(null)
+ const focusTarget = props.focusField?.[0]
useEffect(() => {
- const field = props.focusField?.[0]
- if (!field || !rootRef.current) return
- const el = rootRef.current.querySelector<HTMLElement>(`[data-field="${CSS.escape(field)}"]`)
+ if (!focusTarget || !rootRef.current) return
+ const el = rootRef.current.querySelector<HTMLElement>(`[data-field="${CSS.escape(focusTarget)}"]`)
el?.scrollIntoView({ block: 'center' })
el?.focus()
- }, [props.focusField])
+ }, [focusTarget])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const field = props.focusField?.[0] | |
| if (!field || !rootRef.current) return | |
| const el = rootRef.current.querySelector<HTMLElement>( | |
| `[data-field="${CSS.escape(field)}"]`, | |
| ) | |
| const el = rootRef.current.querySelector<HTMLElement>(`[data-field="${CSS.escape(field)}"]`) | |
| el?.scrollIntoView({ block: 'center' }) | |
| el?.focus() | |
| }, [props.focusField]) | |
| const rootRef = useRef<HTMLDivElement>(null) | |
| const focusTarget = props.focusField?.[0] | |
| useEffect(() => { | |
| if (!focusTarget || !rootRef.current) return | |
| const el = rootRef.current.querySelector<HTMLElement>(`[data-field="${CSS.escape(focusTarget)}"]`) | |
| el?.scrollIntoView({ block: 'center' }) | |
| el?.focus() | |
| }, [focusTarget]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/index.tsx` around lines 186 - 192, Update the
deep-link focus effect around props.focusField to depend on the first field
value rather than the focusField array identity, while preserving the existing
lookup and scroll behavior. Also change the focus target from the non-focusable
[data-field] wrapper to its inner control, or make the wrapper programmatically
focusable with tabIndex={-1}, so focus reliably lands on the field.
| {SETTINGS_FIELDS.map((f) => { | ||
| const set = typeof settings[f.key] === 'number' | ||
| const effective = set ? (settings[f.key] as number) : f.defaultValue | ||
| const scale = 'scale' in f ? f.scale : undefined | ||
| const display = scale === 'minutes' ? msToMinutes(effective) : effective | ||
| return ( | ||
| <div key={f.key}> | ||
| <label className="llmr-cfg-label" htmlFor={`llmr-set-${f.key}`}> | ||
| {f.label} | ||
| </label> | ||
| <input | ||
| id={`llmr-set-${f.key}`} | ||
| className="llmr-cfg-input" | ||
| inputMode="decimal" | ||
| value={set ? String(display) : ''} | ||
| placeholder={String(scale === 'minutes' ? msToMinutes(f.defaultValue) : f.defaultValue)} | ||
| onChange={(e) => { | ||
| const next = { ...settings } | ||
| const n = Number(e.target.value) | ||
| if (e.target.value === '' || Number.isNaN(n)) { | ||
| delete next[f.key] | ||
| } else { | ||
| next[f.key] = scale === 'minutes' ? minutesToMs(n) : n | ||
| } | ||
| commit({ settings: next }) | ||
| }} | ||
| /> | ||
| <div className="llmr-cfg-echo">{set ? `= ${f.echo(effective)}` : `default · ${f.echo(effective)}`}</div> | ||
| <FieldError message={errorAt(props.errors, 'settings', f.key)} /> | ||
| </div> | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The minute conversion breaks decimal input and mangles existing values.
The input is fully controlled from the stored millisecond value. Each keystroke converts through minutesToMs and back through msToMinutes. Intermediate decimal states cannot survive that round trip.
Reproduction:
- The operator types
1in "stream timeout (min)". The form stores60000. The input shows1. - The operator types
.. The value is1..Number('1.')is1. The form stores60000. The re-render sets the input back to1. The period disappears.
The operator therefore cannot enter 1.5 or 0.5. Sub-minute timeouts become unreachable, even though inputMode="decimal" advertises decimals.
The same round trip also harms existing configurations. A stored 100000 renders as 1.6666666666666667 in the input.
Hold the raw text in local state per field. Convert to milliseconds on change. Re-derive the text from the stored value only when the field is not focused.
🐛 Sketch of a draft-text approach
+ // Raw text per settings field: the ms round trip destroys intermediate
+ // decimal states such as "1." while the operator is still typing.
+ const [drafts, setDrafts] = useState<Record<string, string>>({}) const display = scale === 'minutes' ? msToMinutes(effective) : effective
+ const text = drafts[f.key] ?? (set ? String(display) : '')
return (
<div key={f.key}>
<label className="llmr-cfg-label" htmlFor={`llmr-set-${f.key}`}>
{f.label}
</label>
<input
id={`llmr-set-${f.key}`}
className="llmr-cfg-input"
inputMode="decimal"
- value={set ? String(display) : ''}
+ value={text}
placeholder={String(scale === 'minutes' ? msToMinutes(f.defaultValue) : f.defaultValue)}
+ onBlur={() => setDrafts((d) => { const next = { ...d }; delete next[f.key]; return next })}
onChange={(e) => {
+ setDrafts((d) => ({ ...d, [f.key]: e.target.value }))
const next = { ...settings }
const n = Number(e.target.value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/index.tsx` around lines 255 - 288, Update the
SETTINGS_FIELDS input handling to maintain per-field raw draft text locally,
preserving intermediate decimal values such as “1.” while focused. Convert valid
input to milliseconds for commit, but only derive the displayed text from stored
values when the field is not focused; retain existing values such as 100000
without unnecessary minute round-trip rewriting.
| {probe.trim() ? ( | ||
| <div className="llmr-cfg-echo"> | ||
| {winner === null | ||
| ? 'no heuristic match — falls through to the default provider' | ||
| : `row ${winner + 1} → ${heuristicRows[winner].provider}`} | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Announce the probe result and use the provider display name.
Two small improvements for this echo block:
- The result text renders the raw provider id. Every other surface in this form uses
providerDisplayName. Use it here too. - The block appears and updates without an ARIA live region. A screen-reader user who types in the probe input receives no announcement. Add
aria-live="polite".
🐛 Proposed fix
{probe.trim() ? (
- <div className="llmr-cfg-echo">
+ <div className="llmr-cfg-echo" aria-live="polite">
{winner === null
? 'no heuristic match — falls through to the default provider'
- : `row ${winner + 1} → ${heuristicRows[winner].provider}`}
+ : `row ${winner + 1} → ${providerDisplayName(
+ heuristicRows[winner].provider,
+ liveById.get(heuristicRows[winner].provider)?.display_name,
+ )}`}
</div>
) : null}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {probe.trim() ? ( | |
| <div className="llmr-cfg-echo"> | |
| {winner === null | |
| ? 'no heuristic match — falls through to the default provider' | |
| : `row ${winner + 1} → ${heuristicRows[winner].provider}`} | |
| </div> | |
| ) : null} | |
| {probe.trim() ? ( | |
| <div className="llmr-cfg-echo" aria-live="polite"> | |
| {winner === null | |
| ? 'no heuristic match — falls through to the default provider' | |
| : `row ${winner + 1} → ${providerDisplayName( | |
| heuristicRows[winner].provider, | |
| liveById.get(heuristicRows[winner].provider)?.display_name, | |
| )}`} | |
| </div> | |
| ) : null} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/index.tsx` around lines 319 - 325, Update the
probe echo block rendered when probe.trim() is truthy: add aria-live="polite" to
announce changes, and replace heuristicRows[winner].provider with the
corresponding providerDisplayName value used elsewhere in the form while
preserving the existing no-match message and row numbering.
| <button | ||
| type="button" | ||
| className="llmr-cfg-remove" | ||
| aria-label={`move heuristic ${i + 1} up`} | ||
| disabled={i === 0} | ||
| onClick={() => commit({ routing_heuristics: moveItem(heuristics, i, i - 1) })} | ||
| > | ||
| ↑ | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="llmr-cfg-remove" | ||
| aria-label={`move heuristic ${i + 1} down`} | ||
| disabled={i === heuristics.length - 1} | ||
| onClick={() => commit({ routing_heuristics: moveItem(heuristics, i, i + 1) })} | ||
| > | ||
| ↓ | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keyboard reordering stops after the row reaches a boundary.
The move buttons use index keys, so React reuses the same DOM button after a reorder. Focus stays at the same screen position instead of following the moved row.
Two consequences for a keyboard operator:
- After one press of "move heuristic 2 up", focus remains on the up button of row 2, which is now a different rule. A second press moves the wrong row.
- When a row reaches position 1, the up button it now owns becomes
disabled. The browser drops focus todocument.body. Tab order restarts from the top of the page.
Move focus to the button that now belongs to the moved row after the commit, or keep focus on a wrapper that never becomes disabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/index.tsx` around lines 359 - 376, Update the
heuristic reorder controls in the configuration component so keyboard focus
follows the moved heuristic after commit, rather than remaining on a reused
index-keyed button or being lost when the button becomes disabled. Use a stable
row identity and ref/focus handling around the moveItem/commit flow, preserving
the existing boundary disabling and ordering behavior.
| const refresh = async () => { | ||
| try { | ||
| const raw = await host.iii.trigger<unknown>('router::provider::list', {}) | ||
| if (!cancelled) setLive(parseProviderList(raw)) | ||
| } catch { | ||
| if (!cancelled) setLive([]) | ||
| } | ||
| } | ||
| void refresh() | ||
|
|
||
| <label className="llmr-cfg-label" htmlFor={`llmr-${id}-max-tokens`}> | ||
| max tokens | ||
| </label> | ||
| <input | ||
| id={`llmr-${id}-max-tokens`} | ||
| className="llmr-cfg-input" | ||
| inputMode="numeric" | ||
| value={typeof slice.max_tokens === 'number' ? String(slice.max_tokens) : ''} | ||
| placeholder="provider default" | ||
| onChange={(e) => { | ||
| const n = Number(e.target.value) | ||
| set( | ||
| 'max_tokens', | ||
| e.target.value === '' || Number.isNaN(n) ? undefined : n, | ||
| ) | ||
| }} | ||
| /> | ||
| const localFn = `iii::llm-router-ui::providers-changed::${instance}` | ||
| const boundFn = `${localFn}::${host.iii.browserId}` | ||
| const off = host.iii.on(localFn, () => { | ||
| void refresh() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep provider refresh failures and stale responses distinguishable.
The refresh path currently maps rejected or malformed responses to [], making unavailable runtime data look like a successful empty provider list and producing misleading per-provider statuses. The request also has no timeout, so a hung engine can leave all statuses unknown indefinitely, and overlapping refreshes can resolve out of order and overwrite newer data. Track unavailable/error state separately, bound the request, and accept only the latest response.
📍 Affects 2 files
llm-router/ui/src/configuration/index.tsx#L438-L452(this comment)llm-router/ui/src/configuration/provider-cards.ts#L62-L71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/index.tsx` around lines 438 - 452, Update the
refresh flow around refresh and its provider state to track a separate load
error, preserve the distinction between an empty result and a failed
router::provider::list call, and expose a concise retry hint in the form. Race
host.iii.trigger against a defined timeout so stalled requests settle, and add a
monotonic refresh sequence guard that ignores responses and errors from older
overlapping calls while retaining the existing unmount cancellation behavior.
Apply the same fix in `@llm-router/ui/src/configuration/provider-cards.ts` around
lines 62 - 71: Covers the status mapping that cannot distinguish unavailable
runtime data from a successful empty list.
| return () => { | ||
| cancelled = true | ||
| off() | ||
| try { | ||
| unreg?.() | ||
| } catch { | ||
| // SDK already disposed. | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A throw from off() skips unreg().
The cleanup calls off() outside the try block. unreg?.() runs inside a try block. If the SDK is already disposed and off() throws, React never reaches unreg?.(). The router::provider::changed registration then leaks for the remaining session.
Guard both calls.
🐛 Proposed fix
return () => {
cancelled = true
- off()
try {
+ off()
+ } catch {
+ // SDK already disposed.
+ }
+ try {
unreg?.()
} catch {
// SDK already disposed.
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return () => { | |
| cancelled = true | |
| off() | |
| try { | |
| unreg?.() | |
| } catch { | |
| // SDK already disposed. | |
| } | |
| } | |
| return () => { | |
| cancelled = true | |
| try { | |
| off() | |
| } catch { | |
| // SDK already disposed. | |
| } | |
| try { | |
| unreg?.() | |
| } catch { | |
| // SDK already disposed. | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/index.tsx` around lines 463 - 471, Update the
cleanup callback to guard both `off()` and `unreg?.()` within the same
error-handling flow, ensuring `unreg?.()` is still attempted when `off()` throws
while preserving the existing disposed-SDK handling.
| onChange={(e) => { | ||
| const n = Number(e.target.value) | ||
| set('max_tokens', e.target.value === '' || Number.isNaN(n) ? undefined : n) | ||
| }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'max_tokens|Number\.isNaN|Number\.isFinite' llm-router/ui/srcRepository: iii-hq/workers
Length of output: 4405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- provider card context ---'
sed -n '1,230p' llm-router/ui/src/configuration/provider-card.tsx
printf '%s\n' '--- related configuration types and setters ---'
rg -n -C 5 'type JsonValue|JsonValue|function set|const set|set\(' llm-router/ui/src/configuration llm-router/ui/src | head -n 300
printf '%s\n' '--- max_tokens schemas and serialization boundaries ---'
rg -n -C 6 'max_tokens|JSON\.stringify|serialize|JSON value|JsonValue' llm-router --glob '!**/node_modules/**' | head -n 400Repository: iii-hq/workers
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- configuration form wiring ---'
sed -n '115,190p' llm-router/ui/src/configuration/index.tsx
sed -n '300,390p' llm-router/ui/src/configuration/index.tsx
printf '%s\n' '--- JsonValue and form API declarations in tracked files ---'
rg -n -C 4 'ConfigFormProps|export .*JsonValue|type JsonValue|interface JsonValue|onChange\(next' . --glob '!**/node_modules/**' --glob '!**/target/**' --glob '!**/dist/**' | head -n 240
printf '%s\n' '--- focused max_tokens schema context ---'
sed -n '145,168p' llm-router/tests/golden/schemas/router.provider.register.json
sed -n '114,130p' llm-router/tests/golden/schemas/router.provider.resolve.json
printf '%s\n' '--- JavaScript conversion and JSON behavior ---'
node - <<'JS'
for (const text of ['', 'Infinity', '-Infinity', 'NaN', '1.5', '-1', '0x10', ' 42 ']) {
const n = Number(text)
console.log(JSON.stringify({ text, number: String(n), isNaN: Number.isNaN(n), isFinite: Number.isFinite(n), json: JSON.stringify({ max_tokens: n }) }))
}
JSRepository: iii-hq/workers
Length of output: 25516
Validate max_tokens as a non-negative integer.
Number.isFinite rejects Infinity, but it still accepts values such as 1.5 and -1. The schema requires a uint64. Reject values unless Number.isInteger(n) && n >= 0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/src/configuration/provider-card.tsx` around lines 198 - 201,
Update the max_tokens handler in the provider card’s onChange callback to accept
a value only when n is a finite, non-negative integer, preserving undefined for
empty or invalid input. Replace the current NaN-only validation with the
uint64-compatible Number.isInteger(n) && n >= 0 check.
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-loaded { | ||
| color: var(--color-ok); | ||
| background: var(--color-ok-muted); | ||
| } | ||
|
|
||
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-not-loaded { | ||
| color: var(--color-warn); | ||
| background: var(--color-warn-muted); | ||
| } | ||
|
|
||
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-not-connected { | ||
| color: var(--color-alert); | ||
| background: var(--color-alert-muted); | ||
| } | ||
|
|
||
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-overridden { | ||
| color: var(--color-accent); | ||
| background: var(--color-accent-muted); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a style for the unknown provider status.
providerRuntimeStatus in llm-router/ui/src/configuration/provider-cards.ts returns unknown while live is null. useProviderRuntime initialises live to null, so every provider carries the unknown status on first paint, until router::provider::list resolves.
This block styles is-loaded, is-not-loaded, is-not-connected, and is-overridden. It has no unknown variant. The base .llmr-cfg-status rule sets only padding, font, and radius. The badge therefore renders with no colour and no fill during the initial load.
Add a neutral variant.
🎨 Proposed addition
+[data-iii-ui='llm-router'] .llmr-cfg-status.is-unknown {
+ color: var(--color-ink-ghost);
+ background: var(--color-surface-hover);
+}
+
[data-iii-ui='llm-router'] .llmr-cfg-status.is-overridden {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-loaded { | |
| color: var(--color-ok); | |
| background: var(--color-ok-muted); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-not-loaded { | |
| color: var(--color-warn); | |
| background: var(--color-warn-muted); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-not-connected { | |
| color: var(--color-alert); | |
| background: var(--color-alert-muted); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-overridden { | |
| color: var(--color-accent); | |
| background: var(--color-accent-muted); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-loaded { | |
| color: var(--color-ok); | |
| background: var(--color-ok-muted); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-not-loaded { | |
| color: var(--color-warn); | |
| background: var(--color-warn-muted); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-not-connected { | |
| color: var(--color-alert); | |
| background: var(--color-alert-muted); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-unknown { | |
| color: var(--color-ink-ghost); | |
| background: var(--color-surface-hover); | |
| } | |
| [data-iii-ui='llm-router'] .llmr-cfg-status.is-overridden { | |
| color: var(--color-accent); | |
| background: var(--color-accent-muted); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/ui/styles.css` around lines 101 - 119, Add a neutral
`.llmr-cfg-status.is-unknown` variant alongside the existing provider status
rules, using the appropriate neutral foreground and muted background design
tokens. Keep the selector scoped to the `llm-router` UI and preserve the
existing status styles.
1 participant
Summary
Screenshots
Live against Console on
127.0.0.1:3113with Anthropic and OpenAI loaded.Worker configuration
Provider cards
System prompt dialog
Test plan
#/workers/configuration/llm-routeron a fresh-ish engine — cards render for registered providers even with an empty valueset as default${ENV}chip; env placeholder must not look like a saved valuepnpm --filter @iii-workers/llm-router-ui testRefs MOT-4412