fix(webui): query-safe expand state, prefixed search, accessible row toggle

Review fix round (5 Important + 3 folded) against the provider-model
accordion (f9b289f):

1. Manual expand state corruption during an active search. Clicking a
row that was only auto-expanded by the search (never manually opened)
read the RAW manual flag as false and added it to manuallyExpanded -
invisible immediately (already showing via auto-expand) but wrongly
left expanded after the search cleared. Fixed with a two-set design in
a new, Vue-free module (webui/src/lib/provider-expand.ts):
manuallyExpanded (the real, cross-search manual state) plus
manuallyCollapsed (an override a click can set to suppress an
auto-expand while a query is active). toggleProviderExpand now flips
against the caller-computed EFFECTIVE current state, not the raw
manual flag. manuallyCollapsed clears whenever the query becomes empty
(watched, not just the clear button, so backspacing to empty resets it
too). Verified two ways: a throwaway Node assertion script (node
--experimental-strip-types, no test-runner dependency added)
exercises the exact review-described sequence against the plain
module; the identical sequence was then reproduced live in a real
browser and screenshotted at each step.

2. Search matched raw model ids while chips display and copy the
prefixed routable id - copying a chip and pasting it into search found
nothing. Fixed: matching is now against routableModelId(p.name, m) for
both the provider-level filter and the per-provider visible-model
filter - strictly more permissive than the bare id. Verified live:
searching the full "openai/gpt-4o-mini" string now matches.

3. role="button" on a <tr> broke table AT semantics - overriding a
<tr>'s row semantics to "button" strips it from the table's row/column
structure for a screen reader. Moved the interactive control to a real
<button type="button"> inside the first cell (the chevron+name span);
the row's own @click stays as a click-anywhere-in-row convenience, and
the button needs no click handler of its own - its native click bubbles
up to the row's listener, so exactly one place ever runs the toggle.
Added a visible focus style matching the existing shadcn-vue
focus-visible convention used elsewhere in this panel - verified live,
keyboard Tab produces a clearly visible ring. aria-controls is set only
while a provider is expanded, never pointing at an id that does not
yet exist (the content row stays v-if, lazy) - verified live via a
direct DOM check. Provider names are interpolated into that id
unescaped, not sanitized in JS - providers.go's configNamePattern
(charset [a-zA-Z0-9._-]) already rejects any character an HTML
id/attribute value would need escaped, before this endpoint can ever
see one.

4. DRY: providerState.knownIDs() and snapshot() independently rebuilt
the identical explicit-union-discovered merge-and-sort. Extracted
knownIDsLocked() (no locking of its own - caller must already hold
st.mu), consumed by both knownIDs() (takes the lock itself) and
snapshot() (already holds the lock, calls the lock-free variant
directly - a call to the locking knownIDs() from inside snapshot would
deadlock the non-reentrant mutex). snapshot()'s own doc comment
rewritten to describe the actual shared-helper shape instead of the
now-removed duplication it used to explain away.

5. (folded) Corrected a wrong claim in OverviewView.vue's own "why not
Accordion/Collapsible" doc comment: a <div> inside a <tbody> does NOT
get reparented by the browser's HTML parser ("foster parenting") in
this app - Vue renders entirely via DOM APIs, never by parsing an HTML
string, so the parser's foster-parenting algorithm (which only runs
building a DOM tree from a token stream) never executes here. What
actually breaks is CSS table layout: CSS2.1 17.2.1's anonymous-table-
object generation misbehaves around a block box sitting directly
inside a table-row-group box. The comment now says this.

6. (folded) Clipboard fallback hardening: ModelChip.vue's doc comment
now states the select-text fallback is the ONLY path a click ever
takes on a plain-HTTP admin deployment (Clipboard API needs a secure
context), not a rare edge case. A genuine copy failure now sets a
distinct "failed" state (destructive-colored icon, "Copy failed"
title, auto-resets) instead of bailing silently.

Regenerated via make admin-ui; admin_assets_gen.go = 850,968 bytes.
Gate: go build/gofmt/vet/fieldalignment clean, go test -count=1 ok
6.4s, go test -race ok 7.6s, golangci-lint clean (1 pre-existing
unrelated resp.go finding), make yaegi-check OK, npm run build clean,
make integration ok 45.0s (needed 3 attempts - TestUsersFileHotReload
is a pre-existing flake, now observed at least 3 times across this
body of work, unrelated to this diff; flagged for follow-up, not
fixed here).
This commit is contained in:
2026-08-21 10:42:48 +01:00
parent f9b289f1ea
commit b39d4e92c9
5 changed files with 1062 additions and 893 deletions
+26 -33
View File
@@ -68,11 +68,14 @@ func (st *providerState) hasModel(id string) bool {
return st.explicit[id] || st.discovered[id]
}
// knownIDs returns the sorted union of this provider's explicit and
// discovered model ids.
func (st *providerState) knownIDs() []string {
st.mu.Lock()
defer st.mu.Unlock()
// knownIDsLocked returns the sorted union of st.explicit and
// st.discovered. It takes no lock itself — the caller must already hold
// st.mu — so both knownIDs (below) and snapshot (which needs the
// identical merge inside its own, already-held, critical section
// alongside lastRefresh/lastErr) can share this one implementation
// without knownIDs' own st.mu.Lock() double-locking (sync.Mutex is not
// reentrant) when called from inside snapshot.
func (st *providerState) knownIDsLocked() []string {
set := make(map[string]bool, len(st.explicit)+len(st.discovered))
for id := range st.explicit {
set[id] = true
@@ -88,6 +91,14 @@ func (st *providerState) knownIDs() []string {
return ids
}
// knownIDs returns the sorted union of this provider's explicit and
// discovered model ids.
func (st *providerState) knownIDs() []string {
st.mu.Lock()
defer st.mu.Unlock()
return st.knownIDsLocked()
}
// tryBeginRefresh reports whether now is far enough past lastRefresh (or
// this is the first refresh) to start a new one, and if so marks the
// provider inFlight so a concurrent caller cannot start a second one. The
@@ -131,21 +142,15 @@ func (st *providerState) finishRefresh(now time.Time, ids []string, err error) {
// provider-model-accordion task; a caller wanting just the count uses
// len(models)), last refresh time, and last refresh error message.
//
// models is computed in the SAME critical section as lastRefresh/lastErr
// below, not via a separate call to knownIDs() (which does the identical
// explicit∪discovered merge-and-sort): st.mu is a plain sync.Mutex, not
// reentrant, so calling knownIDs() — which takes the same lock — from
// inside this already-locked method would deadlock. The duplication
// mirrors this method's own pre-existing pattern (it already rebuilt the
// same union set as knownIDs() independently, before this change, purely
// for the count) rather than introducing a new one. Keeping the merge
// inside one lock/unlock, rather than one call for the count and a
// second, later call to knownIDs() for the list, also matters
// functionally, not just stylistically: a concurrent finishRefresh
// landing between two separate locked sections could otherwise hand the
// admin dashboard a models list whose length disagrees with a
// separately-read modelCount — reading both from one locked pass makes
// that impossible.
// models comes from knownIDsLocked (this method already holds st.mu, so
// it uses the lock-free variant directly rather than the locking
// knownIDs, which would deadlock reentering the same non-reentrant
// sync.Mutex), read in the SAME critical section as lastRefresh/lastErr
// below — not two separate locked calls. That matters functionally, not
// just stylistically: a concurrent finishRefresh landing between two
// separate locked sections could otherwise hand the admin dashboard a
// models list whose length disagrees with a separately-read modelCount;
// reading all three from one locked pass makes that impossible.
//
// A provider whose discovery is mid-refresh (or has never refreshed
// since a config reload) returns exactly this stale-while-error set —
@@ -156,19 +161,7 @@ func (st *providerState) finishRefresh(now time.Time, ids []string, err error) {
func (st *providerState) snapshot() (models []string, lastRefresh time.Time, lastErr string) {
st.mu.Lock()
defer st.mu.Unlock()
set := make(map[string]bool, len(st.explicit)+len(st.discovered))
for id := range st.explicit {
set[id] = true
}
for id := range st.discovered {
set[id] = true
}
ids := make([]string, 0, len(set))
for id := range set {
ids = append(ids, id)
}
sort.Strings(ids)
return ids, st.lastRefresh, st.lastErr
return st.knownIDsLocked(), st.lastRefresh, st.lastErr
}
// modelRegistry aggregates every configured provider's explicit and