fix(elicitation): allow dialog to be closed via X button and on error#1919
fix(elicitation): allow dialog to be closed via X button and on error#1919nuthalapativarun wants to merge 2 commits intoMCPJam:mainfrom
Conversation
The elicitation dialog's onOpenChange was a no-op so the X button had no effect. Wire it to call cancel so users can always dismiss the dialog. Also fixes the case where the dialog stays stuck after a server-side timeout or API error: - When activeElicitation is already gone (server timed out), clear it so the dialog closes instead of silently returning. - On cancel/decline API errors, clear activeElicitation so the dialog closes rather than leaving users unable to dismiss it. Fixes MCPJam#1604
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull request changes ElicitationDialog so its Dialog Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/components/__tests__/ElicitationDialog.test.tsx`:
- Around line 59-71: The test helper makeRequest constructs a DialogElicitation
but sets timestamp to a number (Date.now()) while DialogElicitation.timestamp is
typed as string; change the timestamp to a string (e.g., new
Date().toISOString() or Date.now().toString()) so the return value of
makeRequest matches the DialogElicitation type and avoids TypeScript errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60c2831a-87e2-4284-a9c0-b5a7c9f4c283
📒 Files selected for processing (3)
mcpjam-inspector/client/src/components/ElicitationDialog.tsxmcpjam-inspector/client/src/components/ToolsTab.tsxmcpjam-inspector/client/src/components/__tests__/ElicitationDialog.test.tsx
`DialogElicitation.timestamp` is typed as `string`; `Date.now()` returns a number which causes a TypeScript type mismatch under strict mode.
f634a54 to
2ac20f3
Compare
Summary
Fixes #1604
The elicitation dialog had two problems that could leave users with an unclosable dialog:
X button did nothing —
onOpenChangewas a no-op (() => {}), so clicking the dialog's X button had no effect. Wired to callcancelso the X button behaves the same as the Cancel button.Dialog stuck after server-side timeout or API error — When the server-side elicitation times out,
activeElicitationon the client is still set. Any subsequent Cancel click would call the API and fail, but thecatchblock only set the error state without clearingactiveElicitation, leaving the dialog permanently open. Fixed by clearingactiveElicitationon cancel/decline API errors, and also whenactiveElicitationis already gone (stale state after server timeout).Changes
ElicitationDialog.tsx:onOpenChange={(open) => { if (!open) handleResponse("cancel"); }}ToolsTab.tsx: ClearactiveElicitationon cancel/decline in catch block; clear it when activeElicitation is null at entryTesting
Unit tests: 4 new tests in
ElicitationDialog.test.tsx, all ToolsTab tests (17) still pass.