fix: restrict keyboard event handling to modal elements only#6408
fix: restrict keyboard event handling to modal elements only#6408bijin-bruno merged 2 commits intousebruno:mainfrom
Conversation
WalkthroughThe Modal component's document-level keydown handler now ignores non-ESC keys when the event target is outside the modal; ESC handling remains global. Enter/submit behavior is preserved but only triggers when the key event originates from inside the modal. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5–10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
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: 0
🧹 Nitpick comments (1)
packages/bruno-app/src/components/Modal/index.js (1)
83-112: Enter key handler correctly respects disabled state; alignuseEffectdeps withconfirmDisabledThe added
!confirmDisabledguard is the right move and keeps keyboard confirm behavior consistent with the disabled button state. One follow‑up:handleKeydownnow depends onconfirmDisabled, butconfirmDisabledis not in theuseEffectdependency array. If a modal uses a stablehandleConfirmwhile togglingconfirmDisabledduring its lifetime, the keydown listener could keep a staleconfirmDisabledand still fire (or never fire) on Enter.You can keep the behavior robust by including
confirmDisabledin the deps so the listener is re‑bound when it changes:- useEffect(() => { - document.addEventListener('keydown', handleKeydown, false); - return () => { - document.removeEventListener('keydown', handleKeydown); - }; - }, [disableEscapeKey, document, handleConfirm]); + useEffect(() => { + document.addEventListener('keydown', handleKeydown, false); + return () => { + document.removeEventListener('keydown', handleKeydown); + }; + }, [disableEscapeKey, handleConfirm, confirmDisabled]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-app/src/components/Modal/index.js(1 hunks)packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.jspackages/bruno-app/src/components/Modal/index.js
🧬 Code graph analysis (1)
packages/bruno-app/src/components/Modal/index.js (1)
packages/bruno-app/src/components/ResponseExample/CreateExampleModal/index.js (1)
handleConfirm(18-28)
⏰ 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). (6)
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (1)
packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js (1)
18-34: Prompt validation andconfirmDisabledwiring look solidThe
allFilledcomputation with a trimmed non‑empty check, combined withconfirmDisabled={!allFilled}, cleanly prevents submitting when any prompt is blank and keeps the modal confirm button and Enter‑key behavior in sync with the required inputs. This aligns well with the PR’s goal of avoiding empty prompt values being sent from the URL bar.
35e305c to
116672a
Compare
116672a to
9b4c130
Compare
Description
This update refines the keyboard event handling logic by restricting it to modal elements only. Previously, keyboard events may have been triggered globally, which could interfere with other UI elements outside of modals. With this change, keyboard event listeners will only be active within modal contexts, ensuring better user experience and avoiding unintended interactions.
Contribution Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.