fix(ui): reset credential form state when switching providers - #25
Merged
songkuan-zheng merged 1 commit intoMay 28, 2026
Merged
Conversation
The credential modal's provider Select wired onChange to only update `selectedProvider` + write the new value to `custom_llm_provider`. It left every other field's Antd Form state untouched, so values seeded by the previous provider's `default_value` carried over. Most visibly: PR #24 added an `api_base` field to Google AI Studio with a default of `https://generativelanguage.googleapis.com/v1beta`, but because OpenAI is the modal's initial provider, OpenAI's `api_base` default (`https://api.openai.com/v1`) populated Antd Form's state on mount. Switching to Google AI Studio re-rendered ProviderSpecificFields with the Google default, but Antd Form's controlled value still pointed at the OpenAI URL — the user saw OpenAI's URL in the api_base field when adding a Google AI Studio credential. Fix: extract a `resetCredentialFormOnProviderChange` helper that resetFields then restores the provider-agnostic fields (credential_name + custom_llm_provider) so the newly rendered ProviderSpecificFields can apply its own defaults from a clean slate. Same call wires both modals (Add + Edit) since they had the same bug. Test plan: - New unit test exercises the helper with 4 scenarios: (1) clears provider-specific fields, (2) preserves credential_name, (3) writes custom_llm_provider + invokes setSelectedProvider, (4) does NOT touch credential_name when it was unset (avoids spurious "required" validation on a brand-new modal). - Existing AddCredentialModal + EditCredentialModal smoke tests still pass (4 tests). - vitest run src/components/model_add/credential_form_helpers.test.ts AddCredentialModal.test.tsx EditCredentialModal.test.tsx -> 8 passed Note: a direct end-to-end "render modal, click Provider Select, switch to Google AI Studio, assert api_base" test would be more comprehensive but Antd Select's portal/dropdown behavior is unreliable in jsdom — attempted patterns (fireEvent.mouseDown, getByRole combobox, document.body portal query) all timed out finding the dropdown options. Testing the extracted helper directly is the more reliable unit-level surrogate.
3 tasks
songkuan-zheng
added a commit
that referenced
this pull request
May 29, 2026
…31) PROBLEM The upstream BerriAI/litellm flow commits the Next.js dashboard bundle to git at litellm/proxy/_experimental/out/ and the Dockerfile just COPYs it. docker/build_admin_ui.sh exists but is a no-op unless the enterprise_colors.json customization marker is present. For this fork that means: any UI source change in ui/litellm-dashboard/ src/ does NOT reach the runtime image — the image keeps serving whatever was last committed to _experimental/out/ (currently April 2026 upstream version). PR #25 (credential form reset), #27 (401 status-aware redirect v1), and #30 (auth error.type routing v2) all shipped UI source but their deployed effect on docker-built images is zero. The defect was only visible via e2e ("why doesn't the live bundle have my code?"). FIX Add an explicit UI build step in the Dockerfile builder stage that runs `npm ci && npm run build` from current source, then overwrites /app/litellm/proxy/_experimental/out/ with the fresh `out/` directory. node_modules and the build cache are pruned in the same RUN so the builder layer stays slim. The existing build_admin_ui.sh hook is kept as the enterprise customization path (no-op by default). Trade-offs vs the upstream commit-the-bundle approach: + UI source changes ALWAYS flow into the image (no "did I run npm run build?" foot-gun). + PR diffs stay clean — no more 100+ file _experimental/out/ churn per UI PR (the recent UI PRs would have been 685 fewer files). + Git history is honest about what's source vs build artifact. - docker build is 2-3 minutes slower (npm install + next build). - Diverges from upstream's pattern, so future upstream rebases that touch _experimental/out/ will need conflict resolution. Manageable because we're now ignoring that path and rebuilding ourselves. GITIGNORE litellm/proxy/_experimental/out/ is added to .gitignore along with the sibling next.js outputs (.next/, out/, node_modules under ui/litellm-dashboard/). 683 previously-tracked artifact files are removed via `git rm --cached` so the working tree stays usable for local non-docker dev (the files remain on disk) but they no longer add diff noise. LOCAL DEV NOTE Developers running the proxy outside docker need `cd ui/litellm-dashboard && npm run build` once to populate _experimental/out/. CLAUDE.md will be updated in a follow-up to reflect this; for now the comment in .gitignore documents the path. VERIFICATION - `git status` after this commit: clean except untracked _experimental/out/ on disk. - `e2e/tools/proxy build` (which invokes docker buildx against this Dockerfile) successfully produces an image. Tested in a follow-up step by stacking PR #30 onto this branch and verifying the served JS chunks contain the auth_session_expired / auth_permission_denied strings — proving the UI build step actually picks up source changes that were never committed to _experimental/out/.
8 tasks
songkuan-zheng
added a commit
that referenced
this pull request
Jun 4, 2026
…e (Wave 7 — final) (#61) * fix(ui): route 401/403 by backend error.type, heuristic only as fallback (#30) * fix(ui): route 401/403 by backend error.type, heuristic only as fallback Companion to fix/proxy-auth-error-type-taxonomy (PR D1) which gives the backend a structured `error.type` for every wrapped auth failure. This PR makes handleErrorResponse read that field FIRST and dispatch by table lookup, with the existing cookie + marker heuristic preserved as a fallback for: - older backend builds that don't emit the new types yet - `type=auth_error` (generic, backend couldn't classify) - unknown / typo'd / future type strings (graceful degradation) Three-tier decision: 1. errorData.error.type or errorData.type: auth_session_expired / auth_invalid_credentials → REDIRECT_LOGIN expired_key / token_not_found_in_db (legacy) → REDIRECT_LOGIN auth_permission_denied → TOAST key/team/user/org/project/vector_store *_access_denied → TOAST team_member_permission_error → TOAST budget_exceeded → TOAST auth_error or unknown → HEURISTIC (step 2) 2. status-based heuristic (legacy / unknown-type fallback): 401 + no cookie OR has marker → REDIRECT 401 + cookie + no marker → TOAST (safe default for 401-as-403) 403 → TOAST 3. non-auth errors → delegate to legacy handleError Why a static table instead of more code: the action is a pure function of the type. Adding a new auth_* type on the backend (e.g. for a new auth mode) requires one line here, no logic change. Test plan: - vitest run src/components/networking.test.ts → 35 passed - 14 new tests in "handleErrorResponse - type-based auth routing (D1 contract)" describe block: - REDIRECT_LOGIN: auth_session_expired (even with cookie present), auth_invalid_credentials, expired_key (legacy), token_not_found_in_db (legacy) - TOAST: auth_permission_denied (the original bug), key_model_access_denied, team_member_permission_error, budget_exceeded - HEURISTIC fallback: auth_error + cookie present (no redirect), auth_error + cookie absent (redirect), unknown type (heuristic) - Body-shape robustness: top-level type field, string body, no type field Graceful degradation: if D1 ships AFTER this PR, the legacy heuristic keeps the UI working. If D2 ships AFTER D1, the new types just route through the heuristic until D2 lands — no breakage either way. * fix(ui): make legacy handleError also read error.type Live e2e revealed handleErrorResponse was dead code — ~30 existing callers use handleError(errorData) directly, not handleErrorResponse. The new type-aware dispatch table only fired for tests, never for real fetch sites. Make the legacy handleError itself read errorData.error.type first and dispatch via AUTH_ERROR_TYPE_TO_ACTION: REDIRECT_LOGIN (auth_session_expired / auth_invalid_credentials / expired_key / token_not_found_in_db) -> triggerSessionExpiredRedirect() TOAST (auth_permission_denied / *_access_denied / *_permission_error / budget_exceeded) -> do nothing here (preserves legacy "silent for non-session" behavior — caller decides whether to toast) HEURISTIC / missing / unknown -> existing message-marker scan + redirect-on-marker This way every existing caller automatically benefits from D1's backend taxonomy with zero call-site migration. Live e2e (rebuilt docker bundle + actual served chunks): 9/9 cases pass — for each backend-emitted type, the live JS chunk maps it to the expected REDIRECT_LOGIN / TOAST action: C1 no-auth-header -> auth_invalid_credentials -> REDIRECT_LOGIN C2 invalid-bearer -> auth_invalid_credentials -> REDIRECT_LOGIN C3 admin-only -> auth_permission_denied -> TOAST (the bug) C5 empty-bearer -> auth_invalid_credentials -> REDIRECT_LOGIN C6 admin+nokey -> auth_invalid_credentials -> REDIRECT_LOGIN C8 unknown-sk-key -> token_not_found_in_db -> REDIRECT_LOGIN C10 unknown-sk-chat -> token_not_found_in_db -> REDIRECT_LOGIN C11 deleted-key -> token_not_found_in_db -> REDIRECT_LOGIN C12 lowpriv-userlist -> (no type, 403) -> TOAST Test plan: - vitest run src/components/networking.test.ts -> 43 passed (5 new "handleError (legacy) - now also reads error.type" cases covering session_expired / invalid_credentials / token_not_found_in_db -> REDIRECT, permission_denied / budget_exceeded -> no redirect, marker fallback when no type, type=auth_error fallback) - Live e2e: built UI -> copied to litellm/proxy/_experimental/out/ -> e2e/tools/proxy build -> 9/9 contract tests pass against live container * fix(ui): redirect to login on session-expired 401, not on all 401s The dashboard's handleError() only matched the exact string "Authentication Error - Expired Key" to detect session expiry. Three problems with that: 1. String-match is fragile — backend reworded the message (or returned an empty message, after the auth log cleanup) and the redirect stopped firing. Users saw 401 toasts and nothing happened. 2. The single marker missed every other session-loss variant: no auth header, key revoked, invalid token, master-key-required, ... 3. No HTTP status awareness — both "session is gone" (401 with no cookie) and "permission denied for this endpoint" (401 because LiteLLM uses 401 for role mismatch too) hit the same path. The "permission denied" case must NOT bounce the user to login because they're already logged in fine; only this specific call fails. Fix: - Add `handleErrorResponse(response, errorData)` — status-aware, preferred whenever the caller has the Response object: * 401 + no auth cookie OR session-expired marker -> redirect to login * 401 + cookie still present + no expiry marker -> toast, no redirect (this is the role-mismatch case) * 403 -> toast only, never redirect (permission denied) * other status -> delegate to handleError (rate-limited path) - Expand handleError's marker list (SESSION_EXPIRED_SIGNALS) to catch more wording variants. handleError keeps its existing signature for backward compat with the ~30 existing callers; new code should use handleErrorResponse instead. - Extract triggerSessionExpiredRedirect() so the "clear cookies + redirect" mechanics live in one place across both code paths. Test plan: - vitest run src/components/networking.test.ts -> 21 passed - New tests (5): handleErrorResponse with 401-no-cookie, 401-with- expiry-marker, 401-still-valid-cookie, 403, 500 — verifies redirect fires only when the session is actually gone. Companion to backend PR that downgrades these auth failures from ERROR to WARN — together they make 401 both quiet in logs and correctly handled in UI. * fix(ui): redirect to login on 401 token_not_found_in_db (#38) * fix(ui): redirect to login on 401 token_not_found_in_db (and siblings) Two-part fix for the bug where the dashboard never redirects to the login page on 401 errors with `error.type=token_not_found_in_db` (and other AUTH_ERROR_TYPE_TO_ACTION REDIRECT_LOGIN types). Patch 1 — root cause: fetch callsites were calling `handleError(deriveErrorMessage(errorData))`, pre-stringifying the error before `handleError` could read `error.type`. `extractErrorType` explicitly rejects strings, so the type-aware fast path never fired. Now callsites pass the raw `errorData` object; `handleError`'s already-implemented D1 taxonomy dispatch finally takes effect. Affects 178 callsites across `networking.tsx` (163) and `src/app/(dashboard)/hooks/` (15 files). Patch 2 — defense in depth: the string-based fallback `SESSION_EXPIRED_SIGNALS` listed `"Authentication Error - Invalid"` (dash) but the backend's token-not-found path emits `"Authentication Error, Invalid"` (comma). Added the comma variant so the heuristic path also catches it when the structured `type` field is absent (older backends). The existing test `handleError redirects when error.type= token_not_found_in_db` (networking.test.ts:394) already asserted the intended behavior; this PR makes production match the test. Updated `useAccessGroups.test.ts` to reflect the new contract (callers pass the object, not the derived message string). * fix(ui): handle MCP tool call errorData scope after Patch 1 sed sweep The previous commit (Patch 1) bulk-replaced `handleError(errorMessage)` with `handleError(errorData)` across 178 callsites. One callsite — the MCP tool call error path — declared `errorData` inside a `try` block, so the replacement referenced an out-of-scope variable and broke the Next.js TypeScript compile (verified via `next build` in the litellm-e2e Docker image rebuild). Fix: hoist `let errorData: any = null` outside the try; assign inside on successful JSON parse. Pass `errorData ?? responseText` to handleError so the structured-type path still fires when JSON is valid, falling back to the raw text otherwise (which `handleError` already handles via its string-marker heuristic). Verified end-to-end against the e2e Docker image: login → inject fake JWT (valid format, key field points to non-existent virtual key) → navigate to /ui/?page=teams → React fires /team/list and friends → backend returns 401 with `error.type=auth_invalid_credentials` → handleError reads the type → triggerSessionExpiredRedirect → window.location.href to /ui/ → server-side guard sends to /ui/login. Critically, the error message "Authentication Error, LiteLLM Virtual Key expected..." does NOT match any SESSION_EXPIRED_SIGNALS marker, so this redirect path is reachable ONLY via the structured-type channel (Patch 1), confirming the fix's contract. * build(docker): rebuild dashboard UI from source on every image build (#31) PROBLEM The upstream BerriAI/litellm flow commits the Next.js dashboard bundle to git at litellm/proxy/_experimental/out/ and the Dockerfile just COPYs it. docker/build_admin_ui.sh exists but is a no-op unless the enterprise_colors.json customization marker is present. For this fork that means: any UI source change in ui/litellm-dashboard/ src/ does NOT reach the runtime image — the image keeps serving whatever was last committed to _experimental/out/ (currently April 2026 upstream version). PR #25 (credential form reset), #27 (401 status-aware redirect v1), and #30 (auth error.type routing v2) all shipped UI source but their deployed effect on docker-built images is zero. The defect was only visible via e2e ("why doesn't the live bundle have my code?"). FIX Add an explicit UI build step in the Dockerfile builder stage that runs `npm ci && npm run build` from current source, then overwrites /app/litellm/proxy/_experimental/out/ with the fresh `out/` directory. node_modules and the build cache are pruned in the same RUN so the builder layer stays slim. The existing build_admin_ui.sh hook is kept as the enterprise customization path (no-op by default). Trade-offs vs the upstream commit-the-bundle approach: + UI source changes ALWAYS flow into the image (no "did I run npm run build?" foot-gun). + PR diffs stay clean — no more 100+ file _experimental/out/ churn per UI PR (the recent UI PRs would have been 685 fewer files). + Git history is honest about what's source vs build artifact. - docker build is 2-3 minutes slower (npm install + next build). - Diverges from upstream's pattern, so future upstream rebases that touch _experimental/out/ will need conflict resolution. Manageable because we're now ignoring that path and rebuilding ourselves. GITIGNORE litellm/proxy/_experimental/out/ is added to .gitignore along with the sibling next.js outputs (.next/, out/, node_modules under ui/litellm-dashboard/). 683 previously-tracked artifact files are removed via `git rm --cached` so the working tree stays usable for local non-docker dev (the files remain on disk) but they no longer add diff noise. LOCAL DEV NOTE Developers running the proxy outside docker need `cd ui/litellm-dashboard && npm run build` once to populate _experimental/out/. CLAUDE.md will be updated in a follow-up to reflect this; for now the comment in .gitignore documents the path. VERIFICATION - `git status` after this commit: clean except untracked _experimental/out/ on disk. - `e2e/tools/proxy build` (which invokes docker buildx against this Dockerfile) successfully produces an image. Tested in a follow-up step by stacking PR #30 onto this branch and verifying the served JS chunks contain the auth_session_expired / auth_permission_denied strings — proving the UI build step actually picks up source changes that were never committed to _experimental/out/.
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
1 participant
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 credential modal's provider Select left every per-provider Antd Form field's value untouched when switching providers, so values seeded by the previous provider's
default_valuecarried over to the next provider's form.Most visibly: PR #24 added
api_baseto the Google AI Studio credential form withdefault_value = https://generativelanguage.googleapis.com/v1beta. But because OpenAI is the modal's initial provider, OpenAI'sapi_basedefaulthttps://api.openai.com/v1populated Antd Form's controlled state on mount. Switching to Google AI Studio re-rendered ProviderSpecificFields with Google's default, but Antd Form's state still pointed at OpenAI's URL — the user saw OpenAI's URL in the api_base field when adding a Google AI Studio credential.Fix
Extract a
resetCredentialFormOnProviderChangehelper that:credential_name(provider-agnostic user-supplied label).form.resetFields()to clear all per-provider state.credential_nameonly if it was set (avoids spuriously triggering the "required" validation on a brand-new modal).setSelectedProvider(value)and writescustom_llm_providerso the next render ofProviderSpecificFieldsapplies its own defaults from a clean slate.Wired into both
AddCredentialModalandEditCredentialModal(same bug pattern in both).Test plan
credential_form_helpers.test.tscovers 4 scenarios:credential_name: undefinedwhen the name was unsetAddCredentialModal.test.tsx+EditCredentialModal.test.tsxsmoke tests still pass.vitest run credential_form_helpers.test.ts AddCredentialModal.test.tsx EditCredentialModal.test.tsx→ 8 passed.Why not a full UI integration test
The ideal regression test would be "render modal, click Provider Select, switch to Google AI Studio, assert api_base value". Tried three patterns:
fireEvent.mouseDown(getByLabelText("Provider:"))+findByText("Google AI Studio")— same pattern as the working SSOModals.test.tsx but timed out.getByRole("combobox", { name: "Provider:" })+userEvent.click— element not found via that role.container.querySelector(".ant-select-selector")+mouseDown+ portal querydocument.body.querySelectorAll(".ant-select-item-option")— dropdown options never rendered.Antd Select with
showSearchdoesn't reliably open in jsdom. Testing the extracted helper directly is the more reliable unit-level surrogate, and it's actually what you want for the bug we're fixing (the bug IS in the handler logic, not in any UI interaction layer).Tag stack
This is the third in a series of related fixes from this session:
fix(router): resolve model_group_alias for order-based fallback lookup(merged)fix(ui): expose api_base on Google AI Studio credential form(merged) — added the field whose default-value leak is fixed here