-
Notifications
You must be signed in to change notification settings - Fork 157
Dont auto-close add server dialog #999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dont auto-close add server dialog #999
Conversation
WalkthroughThe pull request modifies the Add Server modal component to prevent dismissal through outside interactions. Event handlers are added to the DialogContent element to intercept and disable clicks originating from outside the modal boundaries. This ensures the modal remains open until explicitly closed through intended UI interactions. The modal's internal structure and public API signatures remain unaffected. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/components/connection/AddServerModal.tsx (1)
127-131: Outside‑click suppression aligns with Radix patterns; dual handlers are slightly redundantUsing
onPointerDownOutside/onInteractOutsidewithe.preventDefault()cleanly achieves the goal of preventing accidental outside‑click dismissal while still allowing explicit exits via Cancel and Escape. This is an appropriate fit for a data‑entry modal.You likely only need one of these handlers (commonly
onPointerDownOutsideas per Radix docs), unless you intentionally want to guard against non‑pointer interactions as well; keeping both is harmless but a bit redundant. A brief inline comment noting that this modal intentionally ignores outside interactions would also help future readers.Please double‑check in the running app that:
- Escape still closes the dialog, and
- Other dialogs that should close on outside‑click are unaffected (since this override is local to
AddServerModal).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/connection/AddServerModal.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
client/**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (client/CLAUDE.md)
client/**/components/**/*.{ts,tsx}: Use functional React components with React.FC typing
Implement strict TypeScript types for React component props with Props interface definitions
Follow React 19 patterns including hooks, Suspense boundaries, error boundaries, and concurrent features
Use Radix UI components for Dialog, Dropdown menus, Form controls, and Tooltips
Design responsive layouts using Tailwind breakpoint system, grid layouts, flex containers, and container queries
Ensure accessibility compliance with ARIA attributes, keyboard navigation, focus management, and screen reader support
Files:
client/src/components/connection/AddServerModal.tsx
client/**/{components,hooks}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (client/CLAUDE.md)
Use React hooks (useState, useReducer) for local state management with proper effect cleanup
Files:
client/src/components/connection/AddServerModal.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-24T17:54:18.321Z
Learnt from: CR
Repo: MCPJam/inspector PR: 0
File: client/CLAUDE.md:0-0
Timestamp: 2025-11-24T17:54:18.321Z
Learning: Applies to client/**/components/servers/**/*.{ts,tsx} : Implement MCP server management UI with connection list, status indicators, quick actions, and group management
Applied to files:
client/src/components/connection/AddServerModal.tsx
🧬 Code graph analysis (1)
client/src/components/connection/AddServerModal.tsx (1)
client/src/components/ui/dialog.tsx (1)
DialogContent(133-133)
🔍 Remote MCP Context7
Let me search for more specific information about preventing outside-click dismissal and UX best practices:
Summary of Relevant Context for PR Review
Implementation Details: Preventing Outside-Click Dismissal
To prevent a Dialog from closing when clicking outside, the common approach is to pass onPointerDownOutside={(e) => e.preventDefault()} to the DialogContent component, as documented in Radix UI Primitives.
UX Design Considerations
The PR addresses a legitimate UX concern. It can be very frustrating for users to have entered important information and accidentally clicked outside the dialog, which would close the dialog and lose their changes [::web_search::]. This is particularly relevant for the "add server" modal, which involves form input.
However, there's an important design distinction to understand:
-
Closing a dialog when clicking outside is a very common feature of basic dialogs, as inert content outside an active dialog is typically visually obscured or dimmed so it is difficult to discern, and this functionality is common enough that it should be on by default with an easy way to prevent it [::web_search::].
-
If there's a form in a modal, it's important that the user doesn't click outside the window to close it, and you should offer users a 'Cancel' button as the only way the user can close the window [::web_search::].
-
Important modals that contain forms can only be closed through very deliberate action because closing would break some flow or may cause data loss, and these typically have no X button and cannot be closed by clicking outside the modal [::web_search::].
Implementation Considerations
The implementation approach of using onPointerDownOutside={(e)> e.preventDefault()} was noted as surprising by users, as the DialogContentModal component passes disableOutsidePointerEvents to the DismissableLayer component, making this behavior unintuitive [::web_search::].
Additionally, nothing is worse than feeling trapped in a modal dialog, and users should always have a clear way to exit [::web_search::], so the PR should ensure there are explicit exit options (Cancel/Close button, Escape key) alongside the prevention of outside-click dismissal.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: prettier-and-build
khandrew1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I've accidentally clicked outside the modal and lost my fields one too many times :)