-
Notifications
You must be signed in to change notification settings - Fork 8.2k
fix(code-editor): prevent modal from closing when escape is pressed in search box #10585
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
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a guard in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings, 2 inconclusive)
✅ Passed checks (2 passed)
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)
src/frontend/src/modals/codeAreaModal/index.tsx (1)
175-186: Good approach to prevent unintended modal closure.The logic correctly detects the AceEditor search box visibility and returns early to allow AceEditor to handle the Escape key. The visibility check using
offsetParent !== nullis a well-established pattern.Optional: Scope the querySelector to this modal instance
Currently,
document.querySelector("input.ace_search_field")searches the entire document and could potentially match search inputs from other AceEditor instances if multiple modals or editors exist on the page. While this is unlikely to cause issues (since typically only one modal is open at a time), you could make this more robust by scoping the query to this modal's container if you have a ref available.For example, if you had a modal container ref:
const modalRef = useRef<HTMLDivElement>(null); // In the handler: const searchInput = modalRef.current?.querySelector( "input.ace_search_field" ) as HTMLInputElement;However, given the current architecture and typical usage pattern (one modal at a time), the current implementation should work fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/frontend/src/modals/codeAreaModal/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-11T22:12:46.255Z
Learnt from: namastex888
Repo: langflow-ai/langflow PR: 9018
File: src/frontend/src/modals/apiModal/codeTabs/code-tabs.tsx:244-244
Timestamp: 2025-07-11T22:12:46.255Z
Learning: In src/frontend/src/modals/apiModal/codeTabs/code-tabs.tsx, the inconsistent showLineNumbers setting between Step 1 (false) and Step 2 (true) in the API modal is intentional to prevent breaking the modal height. Step 1 uses showLineNumbers={false} to save vertical space while Step 2 uses showLineNumbers={true} for better readability of longer code.
Applied to files:
src/frontend/src/modals/codeAreaModal/index.tsx
⏰ 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). (41)
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 40/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 39/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 34/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 31/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 37/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 32/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 38/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 27/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 36/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 29/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 33/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 35/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 26/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 21/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 28/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 23/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 24/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 25/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 30/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 22/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 20/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 9/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 18/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 19/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 15/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 17/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 14/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 11/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 13/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 16/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 3/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 12/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 10/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 5/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 7/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 6/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 8/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 4/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 2/40
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 1/40
- GitHub Check: Test Starter Templates
🔇 Additional comments (1)
src/frontend/src/modals/codeAreaModal/index.tsx (1)
188-201: Clean integration with existing modal close logic.The new search box check integrates well with the existing escape key handling. When the search box is not visible, the code proceeds with the original behavior: closing the modal if there are no changes, or showing the confirmation dialog if there are unsaved changes (and the autocomplete popup isn't open).
Problem
Pressing Escape inside the AceEditor search text field was closing the code editor modal, preventing users from closing the search box with Escape.
Solution
Added a check in the modal's escape handler to detect if the AceEditor search box input (
input.ace_search_field) is visible in the viewport. If the search box is visible, the escape handler returns early, allowing AceEditor to handle the Escape key to close the search box instead of closing the modal.Changes
onEscapeKeyDownhandler inCodeAreaModalto check for visible search box input before processing escape keyTesting
Summary by CodeRabbit