Fix escape getting eaten by global event handler #1668
Conversation
|
Warning Rate limit exceeded@esimkowitz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces several modifications across multiple files in an Electron application, focusing on improving event handling, logging, and error management. The changes primarily affect keyboard event processing, webview interactions, and application lifecycle management. Key modifications include enhancing the handling of duplicate keyboard events by introducing a The changes also refine the application's shutdown process, particularly in relation to the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/util/keyutil.ts (1)
213-213: Store minimal event data to avoid potential memory leaks
Storing the entire native event retains references to the entire DOM event object. If only certain fields (liketimeStamp,key,code) are needed, consider storing only those fields to reduce overhead and avoid potential circular references.emain/emain.ts (2)
282-282: Optional: Restrict frequent logging
Intensive console logging (e.g., for each key event) may cause performance overhead in production. Consider using a debug flag or thedebuglibrary to limit logs to development or troubleshooting scenarios.
292-292: Provide rationale for removingpreventDefault()
Commenting oute.preventDefault()means the default behavior will now occur for these key events. If this is the intended behavior, adding a short comment clarifying why could help future maintainability.frontend/app/store/keymodel.ts (2)
187-188: Use descriptive naming for clarity
WhilelastHandledEventis clear enough, consider adding a short comment explaining it tracks the last processed keyboard event to avoid duplicates. This improves clarity for future maintainers.
367-367: Consider log verbosity
Logging the entirewaveEventon every key press can generate extensive logs. Use conditional logging or a debug mode to prevent log clutter in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emain/emain.ts(2 hunks)frontend/app/store/keymodel.ts(3 hunks)frontend/app/view/term/term.tsx(2 hunks)frontend/util/keyutil.ts(1 hunks)
🔇 Additional comments (3)
frontend/app/view/term/term.tsx (1)
437-441: Good integration with app-level key handling
CheckingappHandleKeyDownfirst allows for immediate consumption of globally handled shortcuts. This pattern is consistent and prevents duplicative event processing. Nice job!frontend/app/store/keymodel.ts (2)
190-194: Potential edge case with timestamp checks
UsingtimeStampalone works for most scenarios, but two different events can have the same timestamp if they occur within the same millisecond or are artificially triggered. Keep in mind you could also compare keys or other properties to avoid inadvertently filtering out distinct events.
377-377: Return value clarification
Thereturn false;statement indicates that no global key was handled. This is a logical flow for key dispatch, ensuring that subsequent handlers can process unhandled keys. Looks good!
The terminal keydown handler was set to filter out all key bindings that have a registered global handler, regardless of whether they actually propagated or not. This allowed the global handlers to still work despite the terminal input having precedence, but it also meant that global key bindings that were invalid for the current context would still get eaten and not sent to stdin.
Now, the terminal keydown handler will directly call the global handlers so we can actually see whether or not the global key binding is valid. If the global handler is valid, it'll be processed immediately and stdin won't receive the input. If it's not handled, we'll let xterm pass it to stdin. Because anything xterm doesn't handle gets sent to the globally-registered version of the handler, we need to make sure we don't do extra work to process an input we've already checked. We'll store the last-handled keydown event as a static variable so we can dedupe later calls for the same event to prevent doing double work.