fix(ui): make the admin model form actually submit its request - #228
Merged
Merged
Conversation
`#model-confirm` is wired through a click listener that already applies the busy
state -- `addEventListener("click", (e) => withLoading(e.currentTarget, confirmModel))`
-- and `confirmModel` then wrapped its own request in a *second* `withLoading`.
`withLoading` opens with `if (!btn || btn.dataset.loading) return;`, and the outer
wrapper has already set `btn.dataset.loading = "1"` when the click was dispatched,
so the inner call returns immediately: the request was never sent. Adding and
editing a model were both dead, and the button silently went back to its idle
label after ~320ms, which is why it looked like nothing happened.
Both wrappers and the listener come from 98a7a9b (#97), the commit that introduced
the admin models tab, so this has never worked.
Fix: drop the inner wrapper and keep the request inline, mirroring its sibling
`confirmDept` (same admin pane, same listener pattern) -- the loading state still
comes from the listener, so double-click protection and the spinner are unchanged.
The other handlers were checked: `confirmTopup` / `confirmRaise` / `confirmDept`
call the API directly, while `commitNewKey` owns its `withLoading` because its
listener does not wrap it. `confirmModel` was the only one with both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The admin Models tab could never add or edit a model: clicking 确认 (Confirm) validated the form and then silently did nothing — no request was ever sent, and the button went back to its idle label after ~320 ms.
Root cause: the button is wired through a click listener that already applies the busy state —
— and
confirmModelwrapped its own request in a secondwithLoading:The outer wrapper sets
btn.dataset.loading = "1"when the click is dispatched, so the inner call hits the guard and returns immediately:fn(theapi.post/api.patch) is never reached. Both wrappers and the listener come from the same commit —98a7a9b(#97, "model info CRUD") — so this has never worked since the tab was introduced.Fix: drop the inner wrapper and keep the request inline, mirroring the sibling
confirmDept(same admin pane, same listener pattern). The busy state still comes from the listener, so double-click protection and the spinner are unchanged; pressing Enter in the provider/model fields (which calls.click()) goes through the same path.The other handlers were checked for the same shape:
confirmTopup/confirmRaise/confirmDeptcall the API directly because their listeners wrap them, andcommitNewKeyowns itswithLoadingbecause its listener does not wrap it.confirmModelwas the only handler with both.Related Issue
Changes
ui/js/app.js:confirmModelno longer nestswithLoading; the request (POSTon create /PATCHon edit) is issued directly, with a comment stating why the nesting is fatal here.ui/index.html: bump theapp.jscache-bust (?v=20260914-5).Tests
cargo test— 235 passed, 0 failed (unchanged: no Rust file touched)cargo fmt --check— cleancargo clippy --all-targets -- -D warnings— cleannode --check ui/js/app.js— cleanNew unit tests — not applicable: the repo has no JS test infrastructure and this behaviour is not reachable from
cargo test. Verified with a real-DOM harness instead (jsdom bootingui/index.html+ the four real scripts, onlyfetchstubbed, driving the real buttons/forms and asserting the request that the click produced):B6the confirm button is disabled +loadingimmediately after the clickB7edit path emitsPATCH /api/admin/models/203(none)B8create path emitsPOST /api/admin/models(none)A/B (
tmp/c2110_ab.py, baseline read fromorigin/main):{B7, B8}{B7, B8}(it is the pre-change hunk — a self-consistency check){B6}So the instrument rejects both the pre-change tree and the wrong repair, on disjoint checks; the unfiltered control checks (
A2/B2/C2) stay green in every leg.Checklist
fix/)