Skip to content

refactor(ui): consolidate Add/Edit credential modals into one CredentialModal - #32572

Merged
ryan-crabbe-berri merged 5 commits into
litellm_internal_stagingfrom
litellm_dedup_credential_modal
Jul 18, 2026
Merged

refactor(ui): consolidate Add/Edit credential modals into one CredentialModal#32572
ryan-crabbe-berri merged 5 commits into
litellm_internal_stagingfrom
litellm_dedup_credential_modal

Conversation

@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor

Relevant issues

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Screenshots / Proof of Fix

This is a behavior-preserving consolidation of two UI modals, so there is no new user-facing behavior; the goal is that Add and Edit credential still work exactly as before. Two ways to confirm

Automated regression coverage: the merged CredentialModal.test.tsx renders both modes and pins the differences that used to live in the two separate components

cd ui/litellm-dashboard && npx vitest run src/components/model_add/CredentialModal.test.tsx
 ✓ src/components/model_add/CredentialModal.test.tsx (4 tests)

 Test Files  1 passed (1)
      Tests  4 passed (4)

Manual check in the running dashboard

  1. Go to the models area and open the Credentials tab at http://localhost:4000/ui/?page=llm-credentials
  2. Click Add Credential: the modal title reads "Add New Credential", the credential name field is empty and editable, picking a provider swaps in that provider's fields, and Save creates the credential
  3. On an existing credential click the edit icon: the modal title reads "Edit Credential", the name is prefilled and disabled, the provider and its stored values are populated, and Save updates the credential

Type

🧹 Refactoring

Changes

AddCredentialModal and EditCredentialModal were about 90% identical. Both rendered the same provider Select, the same ProviderSpecificFields, and the same submit-and-filter logic; they diverged only in the modal title, the submit button label, whether the credential name was prefilled and disabled, and the edit-only effect that seeds the form from an existing credential

They collapse into a single CredentialModal driven by a mode: "add" | "edit" prop plus an optional existingCredential. The two call sites in credentials.tsx pass the mode and their respective submit handler through a unified onSubmit. Behavior is preserved down to the small quirks (edit-only destroyOnHidden, the disabled name field, and the per-mode default provider), so this is a pure dedup with no functional change

That removes roughly 120 lines of duplication and lets the two per-file test files merge into one CredentialModal.test.tsx that covers both modes: add shows an empty editable name and renders provider fields, edit prefills and disables the name. Consolidating the two copies also drops the dashboard no-explicit-any count and collapses two no-restricted-imports suppression entries into one, so the lint baselines ratchet down

…ialModal

AddCredentialModal and EditCredentialModal were ~90% identical: the same
provider select, ProviderSpecificFields, and submit/filter logic, differing
only in title, button text, edit-mode prefill, and the disabled credential
name. Replace both with a single CredentialModal driven by a mode: 'add' |
'edit' prop, and point the two call sites in credentials.tsx at it.

Removes ~120 lines of duplication and drops the no-explicit-any and
no-restricted-imports baselines. The two per-file tests merge into one
CredentialModal.test.tsx covering both modes (add: editable empty name;
edit: prefilled, disabled name; provider fields render).
@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR consolidates AddCredentialModal and EditCredentialModal — which were ~90% identical — into a single CredentialModal driven by a mode: "add" | "edit" prop. The refactor is behavior-preserving: all mode-specific branches use the isEdit flag consistently, form seeding switches from useEffect/setFieldsValue to form-level initialValues (safe because both call sites conditionally mount/unmount the component on open/close), and destroyOnHidden is retained for edit mode only.

  • CredentialModal.tsx: The key behavioral differences from the two originals are all correctly handled — disabled={isEdit} on the name field (fixing the old truthy-name bug), destroyOnHidden={isEdit}, and initialValues derived from existingCredential at mount time.
  • CredentialModal.test.tsx: Merged test suite covers both modes, including a dedicated case that confirms the name field is disabled by mode, not by the credential name's truthiness.
  • eslint-metrics.json: no-explicit-any drops by 4 as expected; local/no-large-inline-object-arg rises by 7, attributable to the large mock objects in the new test file.

Confidence Score: 5/5

Safe to merge — this is a pure UI deduplication with no functional changes to credential creation or editing flows.

The consolidation is mechanically sound. Both call sites in credentials.tsx conditionally mount/unmount the modal, so initialValues-based form seeding is applied fresh on every open — the same guarantee the old useEffect approach provided. The isEdit flag drives all mode-dependent branches consistently, and the new test suite explicitly covers the edge cases (including the empty-name-in-edit-mode scenario). No logic was changed, only structure.

No files require special attention.

Important Files Changed

Filename Overview
ui/litellm-dashboard/src/components/model_add/CredentialModal.tsx New unified modal combining AddCredentialModal and EditCredentialModal; uses isEdit for all mode-branching, initialValues for form seeding, and destroyOnHidden preserved for edit mode only — behaviorally equivalent to the two components it replaces.
ui/litellm-dashboard/src/components/model_add/credentials.tsx Call sites updated to use the unified CredentialModal with mode prop and onSubmit; conditional rendering ensures fresh remounts, so initialValues-based form seeding works correctly.
ui/litellm-dashboard/src/components/model_add/CredentialModal.test.tsx New merged test file covers add mode (empty editable name, provider fields) and edit mode (prefill, disabled name, and the "disabled by mode not by name value" edge case that fixed the old bug).
ui/litellm-dashboard/eslint-metrics.json no-explicit-any drops by 4 as expected; local/no-large-inline-object-arg rises by 7, likely from the large mock objects added in CredentialModal.test.tsx.
ui/litellm-dashboard/eslint-suppressions.json Collapses two per-file suppression entries (AddCredentialModal + EditCredentialModal) into one for CredentialModal; also removes the react-hooks/set-state-in-effect entry that was needed for the old useEffect in EditCredentialModal.
ui/litellm-dashboard/src/components/model_add/EditCredentialModal.tsx Deleted; functionality fully absorbed by CredentialModal.tsx.

Reviews (3): Last reviewed commit: "refactor(ui): prefill credential form de..." | Re-trigger Greptile

Comment thread ui/litellm-dashboard/src/components/model_add/CredentialModal.tsx
@codspeed-hq

codspeed-hq Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_dedup_credential_modal (71fffe1) with litellm_internal_staging (010b200)1

Open in CodSpeed

Footnotes

  1. No successful run was found on litellm_internal_staging (703327a) during the generation of this report, so 010b200 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The disabled flag on the credential name field was tied to whether
existingCredential?.credential_name is truthy, an artifact of the old
EditCredentialModal. Drive it from the isEdit flag like the rest of the
component so mode='add' with a stray existingCredential can't disable the
field and mode='edit' with an empty name can't leave it editable. Behavior
is unchanged for real call sites; adds a regression test for the edit-with-
empty-name case.
@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai Addressed the P2: the credential name field's disabled state now derives from the mode (isEdit) instead of existingCredential?.credential_name truthiness, matching the rest of the component. Added a regression test for the edit-with-empty-name case.

@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

The fix looks correct. disabled={isEdit} is now derived purely from the mode prop, and the new test case ("disables the name from the mode, not the credential's name value") directly pins the previously buggy path — edit mode with an empty credential_name still disables the field.

The consolidated CredentialModal.test.tsx now covers all four meaningful cases cleanly. LGTM.

@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai re review

…eEffect

The edit-mode form was seeded with an imperative form.setFieldsValue inside
a useEffect that also set React state (setSelectedProvider), an antd anti-
pattern carried over from the old EditCredentialModal. Both call sites mount
the modal fresh with existingCredential already present (conditional && plus
destroyOnHidden), so there is no 'prop arrives after mount' case to handle.

Replace it with antd's declarative initialValues on the Form and a lazy
useState initializer for the provider. Removes the effect, its
react-hooks/set-state-in-effect suppression and exhaustive-deps warning, and
one any cast; behavior is unchanged (edit now shows the real provider on
first paint instead of flashing the default). Existing tests cover prefill
and the disabled name field.
@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai re review

@devin-ai-integration

Copy link
Copy Markdown
Contributor

QA: consolidated CredentialModal (add + edit)

QA'd end to end against a live proxy plus dev server on this branch. Both modes of the merged CredentialModal behave exactly like the two original components, with no functional regression

Add mode (mode="add")

  • Title reads "Add New Credential", credential name field is empty and editable, submit button reads "Add Credential"
  • Selecting a provider swaps in that provider's fields (OpenAI -> Anthropic changes the fields to Upstream API Base and API Key and drops the OpenAI Org ID field)
  • Save creates the credential and shows "Credential added successfully", new row appears in the table

Add modal

Provider swap to Anthropic

Add success

Edit mode (mode="edit", existingCredential passed)

  • Title reads "Edit Credential", credential name is prefilled and disabled, provider and stored values are populated, submit button reads "Update Credential"
  • Save updates the credential and shows "Credential updated successfully", row is retained

Edit modal

Edit success

Walkthrough

Walkthrough recording

One non-blocking note for reviewers: the PR shows a merge conflict against litellm_internal_staging, so it needs a rebase before merge

…itellm_dedup_credential_modal

# Conflicts:
#	ui/litellm-dashboard/eslint-metrics.json
#	ui/litellm-dashboard/eslint-suppressions.json
@ryan-crabbe-berri
ryan-crabbe-berri enabled auto-merge (squash) July 18, 2026 18:20
@ryan-crabbe-berri
ryan-crabbe-berri merged commit 6f4f4f6 into litellm_internal_staging Jul 18, 2026
79 checks passed
@ryan-crabbe-berri
ryan-crabbe-berri deleted the litellm_dedup_credential_modal branch July 18, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants