-
Notifications
You must be signed in to change notification settings - Fork 750
feat: close search dialog with ctrl+f #1807
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
Conversation
Summary of ChangesHello @RohitKushvaha01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience by implementing a common and intuitive keyboard shortcut, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a Ctrl+F shortcut to close the search dialog when the search input is focused. My review points out a potential issue with the implementation where using toggleSearch() can lead to unexpected behavior, such as clearing the search input or not closing the dialog under certain conditions. I've suggested a more direct approach to ensure the dialog closes as intended.
| const { editor, activeFile } = editorManager; | ||
| editor.focus(); | ||
| toggleSearch(); |
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.
Calling toggleSearch() here can lead to unintended side effects. For example:
- If there is text selected in the editor, it might replace the content of the search input instead of closing the search dialog.
- If the search input is not empty and there is no selection in the editor, it will clear the search input.
This is likely not the desired behavior when the user presses Ctrl+F with the intent to close the search dialog. A more direct approach is to call the action that closes the search bar from the actionStack. This ensures that the search dialog is always closed as intended.
Additionally, activeFile is declared but never used.
| const { editor, activeFile } = editorManager; | |
| editor.focus(); | |
| toggleSearch(); | |
| const { editor } = editorManager; | |
| editor.focus(); | |
| actionStack.get("search-bar")?.action(); |
Greptile OverviewGreptile SummaryThis PR adds the ability to close (or toggle) the search dialog by pressing Ctrl+F (or Cmd+F on Mac) when the search input has focus. Implementation DetailsThe change adds a keydown event handler to the search input that:
BehaviorThe actual behavior is more nuanced than just "closing" the search:
This provides an intelligent UX where users can either close the search or update it with newly selected text, depending on context. Code QualityThe implementation follows existing patterns in the codebase and integrates cleanly with the existing Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant SearchInput
participant KeyHandler
participant Editor
participant toggleSearch
participant ActionStack
User->>SearchInput: Press Ctrl+F
SearchInput->>KeyHandler: keydown event
KeyHandler->>KeyHandler: Check (ctrlKey || metaKey) && key === 'f'
KeyHandler->>KeyHandler: preventDefault()
KeyHandler->>Editor: focus()
KeyHandler->>toggleSearch: toggleSearch()
toggleSearch->>Editor: getSelectedText()
Editor-->>toggleSearch: selectedText
alt Search already open
alt selectedText != inputValue
toggleSearch->>SearchInput: Update value with selectedText
toggleSearch->>Editor: find(0, false)
toggleSearch-->>KeyHandler: return (search updated)
else selectedText == inputValue
toggleSearch->>ActionStack: get("search-bar").action()
ActionStack->>toggleSearch: removeSearch()
toggleSearch->>SearchInput: Remove from DOM
toggleSearch-->>KeyHandler: return (search closed)
end
else Search not open
toggleSearch->>SearchInput: Create and show search
toggleSearch->>ActionStack: push search close action
toggleSearch->>SearchInput: focus()
toggleSearch-->>KeyHandler: return (search opened)
end
|
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.
1 file reviewed, 1 comment
| if (e.key === "ArrowUp") { | ||
| if ((e.ctrlKey || e.metaKey) && e.key.toLowerCase() === "f") { | ||
| e.preventDefault(); | ||
| const { editor, activeFile } = editorManager; |
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.
The activeFile variable is destructured but never used in this code block. Consider removing it to keep the code clean.
| const { editor, activeFile } = editorManager; | |
| const { editor } = editorManager; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/handlers/quickTools.js
Line: 88:88
Comment:
The `activeFile` variable is destructured but never used in this code block. Consider removing it to keep the code clean.
```suggestion
const { editor } = editorManager;
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Closes #1427