fix(ui/mcp): do not reset in-flight OAuth resume when create modal mounts closed - #32416
Conversation
Greptile SummaryFixes a regression where
Confidence Score: 5/5Safe to merge — the change is a two-line guard around an existing cleanup block and is covered by a targeted regression test. The fix is minimal and correct: initializing the ref to the prop's initial value means a mount-with-closed never satisfies No files require special attention.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx | Guards the cleanup effect with a previous-visibility ref so it only fires on a real open→closed transition, preventing the post-OAuth-redirect mount from resetting an in-flight token exchange |
| ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx | Adds a focused regression test that verifies (a) mounting with isModalVisible=false does not call reset and (b) a real open→closed transition still does, covering both sides of the guard |
Reviews (1): Last reviewed commit: "fix(ui/mcp): do not reset in-flight OAut..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 13.37%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_completion_simple_message |
4.7 ms | 4.2 ms | +13.37% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing litellm_mcp_create_oauth_resume_reset_fix (8543e6b) with litellm_internal_staging (4b0ac8b)
chore(ui): rebuild dashboard bundle for patch-1.92.0rc1 (#32416)
…unts closed (BerriAI#32416) (cherry picked from commit 7cc6608)
Relevant issues
Regression introduced by #30000
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
UI-only fix, so the proof is the on-create OAuth flow in the Admin UI. Steps to reproduce and verify:
python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml --detailed_debug) and the dashboard dev server (npm run devinui/litellm-dashboard)https://mcp.linear.app/mcp), Authentication = OAuth, flow = Interactive (PKCE)The pairs below capture the create-server modal in the state it lands in after the OAuth redirect returns, before at the merge-base
4b0ac8b352(the buggy cleanup effect) and after at this branch's head8543e6b8fa. Each is rendered from the dashboard dev server driving the realCreateMCPServerwith the redirect remount replayed whileresumeOAuthFlow's token exchange is in flight; only the token exchange and the tools listing are stubbed, so the sole variable between the two columns is this PR's cleanup guardThe OAuth section is where the regression first shows: before, the redirect returns but the mount-time
resetOAuthFlow()discards the exchanged token so the "Token fetched" line never renders; after, the token survives the remount and the confirmation appearsBecause the discarded token leaves
oauthAccessTokennull, Connection Status cannot test the server and stays on its "complete required fields" placeholder before; after, it lists the server as connectedTool Configuration is driven by the same connection, so it stays empty before and populates with the server's tools ready to allowlist after
Type
🐛 Bug Fix
Changes
The create-server modal has a cleanup effect that resets form, tools, and OAuth state whenever
isModalVisibleis false, added in #30000 so a previous server's token cannot bleed into the next add-server session. That effect also runs on the initial mount, which breaks the "Authorize & Fetch Token" flow: after the OAuth redirect returns, the page reloads andCreateMCPServermounts with the modal still closed (the restore effect reopens it a moment later) whileuseMcpOAuthFlow.resumeOAuthFlowis already exchanging the authorization code. The mount-timeresetOAuthFlow()bumps the hook's reset version, so when the exchange resolves the hook discards the token by design (if (resetVersion !== resetVersionRef.current) return). The user sees restored form fields but no token, no Connection Status, no Tool Configuration, and the saved server never receives the per-user credential, forcing a second authorization from the server cardThe fix guards the cleanup so it only fires on a real open to closed transition, tracked with a ref of the previous visibility. A genuine dismissal (cancel, successful create, or a parent flipping the prop) still resets everything, which keeps the #30000 leak fix intact; the post-redirect mount, where the modal starts closed, no longer clobbers the in-flight resume
Regression test: mounting with the modal closed must not call the OAuth hook's
reset(), while an open to closed transition still must. The test fails on the previous code (reset was called on mount) and passes with the guard. The hook-side half of the contract, reset discarding an in-flight exchange, was already pinned byuseMcpOAuthFlow.test.tsxLink to Devin session: https://app.devin.ai/sessions/f03da2725ec94d28b3facf766871b102