feat: Add search functionality to PathInput with keyboard shortcut support #268
Conversation
- Added Kbd and KbdGroup components for keyboard shortcuts in FileBrowserDialog. - Implemented search functionality in PathInput, allowing users to search files and directories. - Updated PathInput to handle file system entries and selection from search results. - Improved UI/UX with better focus management and search input handling.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds OS-detection hook and keyboard UI ( Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,248,255,0.7)
participant User
participant FileBrowserDialog
participant PathInput
participant SearchOverlay
end
User->>FileBrowserDialog: Open dialog
User->>PathInput: Press "/"
PathInput->>SearchOverlay: Open search overlay (focus)
User->>SearchOverlay: Type query
SearchOverlay->>SearchOverlay: Filter `entries` (files & dirs)
User->>SearchOverlay: Select entry
alt entry.isDirectory
SearchOverlay->>PathInput: onSelectEntry(directory)
PathInput->>FileBrowserDialog: handleSelectDirectory(directory.path)
FileBrowserDialog->>FileBrowserDialog: Navigate into directory
else file selected
SearchOverlay->>PathInput: onSelectEntry(file)
PathInput->>FileBrowserDialog: (file selection callback)
end
FileBrowserDialog->>PathInput: Close overlay / update path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Comment |
Summary of ChangesHello @illia1f, 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 significantly enhances the PathInput component by adding a robust search functionality, making it easier and faster for users to navigate and select files or directories. The implementation focuses on a keyboard-first experience, introducing intuitive shortcuts and new reusable UI components for displaying keyboard commands. This change streamlines the file browsing process, particularly within the FileBrowserDialog, by providing quick search access and improved visual consistency. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable search functionality to the PathInput component, complete with keyboard shortcuts, which significantly improves user experience. The implementation is generally well-executed, including the creation of reusable Kbd components. My review highlights a few areas for improvement to enhance correctness, maintainability, and robustness. Specifically, I've pointed out a component type definition that needs correction, an opportunity to simplify a useEffect hook for better readability, and a syntax error in a Tailwind CSS class name that needs fixing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
376-383:navigator.platformis deprecated.
navigator.platformis deprecated and may be removed in future browser versions. Consider usingnavigator.userAgentData?.platformwith a fallback, or a simple heuristic based onnavigator.userAgent.🔎 Proposed fix
<KbdGroup className="ml-1"> <Kbd> - {typeof navigator !== 'undefined' && navigator.platform?.includes('Mac') + {typeof navigator !== 'undefined' && + (navigator.userAgentData?.platform === 'macOS' || + navigator.platform?.includes('Mac')) ? '⌘' : 'Ctrl'} </Kbd> <Kbd>↵</Kbd> </KbdGroup>apps/ui/src/components/ui/path-input.tsx (2)
322-324: Inconsistent keyboard hint styling.This inline
<kbd>element should use the newKbdcomponent for visual consistency across the codebase.🔎 Proposed fix
First, add the import at the top of the file:
+import { Kbd } from '@/components/ui/kbd';Then update the inline kbd:
- <kbd className="hidden sm:inline-block px-1.5 py-0.5 text-[10px] bg-muted rounded border border-border text-muted-foreground"> - ESC - </kbd> + <Kbd className="hidden sm:inline-block">ESC</Kbd>
130-143: Consider using a ref callback instead ofsetTimeoutfor focus management.Using
setTimeout(..., 0)to wait for the DOM is fragile and may fail if the Command component defers its own rendering. A ref callback orrequestAnimationFramecan be more reliable, though for a simple case like this the current approach is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/components/ui/kbd.tsxapps/ui/src/components/ui/path-input.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/ui/src/components/ui/path-input.tsx (3)
apps/ui/src/components/ui/button.tsx (1)
Button(108-108)apps/ui/src/lib/utils.ts (1)
cn(5-7)apps/ui/src/components/ui/breadcrumb.tsx (6)
Breadcrumb(95-95)BreadcrumbList(96-96)BreadcrumbItem(97-97)BreadcrumbPage(99-99)BreadcrumbLink(98-98)BreadcrumbSeparator(100-100)
apps/ui/src/components/ui/kbd.tsx (1)
apps/ui/src/lib/utils.ts (1)
cn(5-7)
⏰ 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: e2e
🔇 Additional comments (5)
apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
256-261: LGTM!The PathInput integration correctly maps directories to the expected
FileSystemEntryformat and properly handles entry selection by navigating only into directories.apps/ui/src/components/ui/path-input.tsx (4)
226-249: LGTM on global keyboard shortcuts.The implementation correctly:
- Uses capture phase to intercept events before other handlers
- Prevents "/" from being typed into input/textarea elements
- Uses
stopPropagation()on Escape to prevent parent modal from closing- Cleans up event listeners properly
251-264: LGTM on click-outside handling.The
mousedownevent is appropriate here as it fires before focus changes, ensuring the search closes before any other element receives focus.
63-67: LGTM!Clean interface definition with appropriate properties for file system entries.
303-350: Search popover implementation looks solid.The Command-based search UI is well-structured with proper accessibility labels, keyboard navigation, and visual distinction between files and directories.
…ce PathInput with autoFocus on CommandInput
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/ui/kbd.tsxapps/ui/src/components/ui/path-input.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ui/src/components/ui/kbd.tsx (1)
apps/ui/src/lib/utils.ts (1)
cn(5-7)
⏰ 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: e2e
🔇 Additional comments (15)
apps/ui/src/components/ui/kbd.tsx (2)
3-16: LGTM! Clean implementation of the Kbd component.The component is properly typed, uses data-slot for styling hooks, and includes comprehensive styling for various use cases including tooltip content.
18-26: LGTM! Type mismatch resolved.The component now correctly uses
React.ComponentProps<'span'>and renders a<span>element, addressing the previous review concerns. The use of<span>is semantically appropriate for groupingKbdcomponents.apps/ui/src/components/ui/path-input.tsx (13)
3-3: LGTM! Appropriate icon additions.The new icons (Search, Folder, File, X) support the search functionality and visual distinction between file types.
14-21: LGTM! Command component imports.These imports are necessary for the new search overlay functionality using the Command component.
63-67: LGTM! Clean FileSystemEntry interface.The interface is well-defined with clear properties for representing file system entries. The
isDirectoryboolean provides necessary distinction between files and folders.
80-91: LGTM! Well-documented prop additions.The new props for search functionality are properly typed and documented with clear JSDoc comments. The optional nature of these props maintains backward compatibility.
100-108: LGTM! Proper state and prop defaults.The component properly destructures the new props with sensible defaults (
searchPlaceholder,entries,onSelectEntry) and adds theisSearchOpenstate for managing search visibility.
201-209: LGTM! Clean entry selection handler.The callback correctly invokes the parent's
onSelectEntryhandler when provided and properly closes the search overlay. Dependencies are correct.
211-234: LGTM! Comprehensive keyboard shortcut handling.The global keyboard shortcuts are well-implemented:
/opens search (excluded when in input fields)Escapecloses search with intentionalstopPropagationto prevent parent modal closure- Uses capture phase for reliable event interception
- Proper dependency array
236-249: LGTM! Standard click-outside pattern.The effect properly closes the search overlay when clicking outside, using
mousedownevent for reliable detection and including proper cleanup.
290-310: LGTM! Clean search overlay implementation.The search popover is properly positioned with good z-index management. The use of
autoFocuson theCommandInput(line 297) addresses previous review feedback and provides a better user experience.
316-334: LGTM! Clean search results rendering.The search results properly distinguish between directories and files using appropriate icons and styling. The use of
CommandList,CommandEmpty, andCommandItemfollows the Command component patterns correctly.
341-349: LGTM! Proper click handling.The container click handler correctly excludes the search open state (line 190) and prevents triggering when clicking buttons or links.
351-404: LGTM! Well-structured breadcrumb view with integrated controls.The breadcrumb display is clean and includes properly positioned search and edit buttons. The search button is appropriately disabled when there are no entries to search (line 390), providing good UX feedback.
405-432: LGTM! Clean input mode implementation.The input view provides a straightforward text input with a go button, proper keyboard handling (Enter/Escape), and appropriate disabled states. All accessibility attributes are in place.
… for better styling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ui/src/components/ui/path-input.tsx (1)
318-331: Consider memoizing the entry list rendering.The entries mapping is not memoized and will re-render on every component render. For directories with many files (hundreds or thousands), this could impact performance.
🔎 Proposed optimization
Memoize the entry items:
+ const entryItems = useMemo( + () => + entries.map((entry) => ( + <CommandItem + key={entry.path} + value={entry.name} + onSelect={() => handleSelectEntry(entry)} + > + {entry.isDirectory ? ( + <Folder className="w-3.5 h-3.5 text-brand-500 mr-2" /> + ) : ( + <File className="w-3.5 h-3.5 text-muted-foreground mr-2" /> + )} + <span className="flex-1 truncate font-mono text-xs">{entry.name}</span> + </CommandItem> + )), + [entries, handleSelectEntry] + ); <CommandList className="scrollbar-styled"> <CommandEmpty>No files or directories found</CommandEmpty> <CommandGroup> - {entries.map((entry) => ( - <CommandItem - key={entry.path} - value={entry.name} - onSelect={() => handleSelectEntry(entry)} - > - {entry.isDirectory ? ( - <Folder className="w-3.5 h-3.5 text-brand-500 mr-2" /> - ) : ( - <File className="w-3.5 h-3.5 text-muted-foreground mr-2" /> - )} - <span className="flex-1 truncate font-mono text-xs">{entry.name}</span> - </CommandItem> - ))} + {entryItems} </CommandGroup> </CommandList>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/components/ui/path-input.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). (1)
- GitHub Check: e2e
🔇 Additional comments (7)
apps/ui/src/components/ui/path-input.tsx (7)
64-68: LGTM!The
FileSystemEntryinterface is well-defined with appropriate fields for representing filesystem items in the search functionality.
81-82: LGTM!The new props are well-designed with appropriate types, sensible defaults, and maintain backward compatibility through optional parameters.
Also applies to: 89-92
202-210: LGTM!The
handleSelectEntrycallback correctly invokes the parent handler and manages the search state. TheuseCallbackdependencies are appropriate.
237-250: LGTM!The click-outside handler correctly detects clicks outside the container and closes the search. The use of
mousedownand the conditional setup based onisSearchOpenfollow best practices.
289-338: LGTM!The search popover implementation is well-structured with proper positioning, focus management (
autoFocuson line 298), and the reusableKbdcomponent (line 312) for keyboard shortcut display.
382-392: LGTM!The search button correctly handles the disabled state when no entries are available and includes proper accessibility attributes.
439-439: LGTM!The exports correctly include the new
FileSystemEntrytype alongside the existing exports.
- Updated keydown event logic to prevent search activation when input fields or contenteditable elements are focused. - Enhanced ESC key handling to ensure parent modal does not close when search is open. - Adjusted dependencies in useEffect to include entries length for better state management.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ui/src/components/ui/path-input.tsx (1)
233-234: Clarify the comment about ESC handling mechanism.The comment states "prevent parent modal from closing when search is open," but it's actually
preventDefault()on line 228 (notstopPropagation()) that achieves this. Using capture phase ensures this handler runs before the parent modal's ESC handler, andpreventDefault()then prevents the default action.Consider updating the comment to accurately reflect the mechanism and whether capture phase is necessary for the "/" shortcut (which doesn't need to prevent other handlers).
🔎 Suggested clarification
- window.addEventListener('keydown', handleGlobalKeyDown, true); // Use capture phase for ESC handling and prevent parent modal from closing when search is open - return () => window.removeEventListener('keydown', handleGlobalKeyDown, true); + // Use capture phase to handle ESC before parent modal handlers. + // preventDefault() on line 228 prevents the parent modal from seeing the ESC key. + window.addEventListener('keydown', handleGlobalKeyDown, true); + return () => window.removeEventListener('keydown', handleGlobalKeyDown, true);Alternatively, if you want both shortcuts to use standard bubble phase (and only preventDefault for ESC):
- window.addEventListener('keydown', handleGlobalKeyDown, true); // Use capture phase for ESC handling and prevent parent modal from closing when search is open - return () => window.removeEventListener('keydown', handleGlobalKeyDown, true); + window.addEventListener('keydown', handleGlobalKeyDown); + return () => window.removeEventListener('keydown', handleGlobalKeyDown);Note: The second approach would allow the parent modal to also close when ESC is pressed (both search and modal close together). Choose based on your desired UX.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/components/ui/path-input.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ui/src/components/ui/path-input.tsx (3)
apps/ui/src/components/ui/button.tsx (1)
Button(108-108)apps/ui/src/lib/utils.ts (1)
cn(5-7)apps/ui/src/components/ui/breadcrumb.tsx (6)
Breadcrumb(95-95)BreadcrumbList(96-96)BreadcrumbItem(97-97)BreadcrumbPage(99-99)BreadcrumbLink(98-98)BreadcrumbSeparator(100-100)
⏰ 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: e2e
🔇 Additional comments (7)
apps/ui/src/components/ui/path-input.tsx (7)
64-68: LGTM! Well-defined interface for filesystem entries.The
FileSystemEntryinterface is clean and appropriately typed for representing files and directories in the search functionality.
81-92: LGTM! New props are well-designed and documented.The new props for search functionality (
searchPlaceholder,entries,onSelectEntry) have appropriate defaults and are properly documented, maintaining backward compatibility.
202-210: LGTM! Entry selection handler is well-implemented.The
handleSelectEntrycallback correctly invokes the parent callback and closes the search overlay, with proper memoization.
237-250: LGTM! Click-outside handler follows best practices.The implementation correctly uses
mousedowninstead ofclickand properly checks containment before closing the search overlay.
289-338: LGTM! Search overlay is well-implemented with good accessibility.The search UI properly uses the Command component pattern with:
- Auto-focus for immediate typing
- Clear visual distinction between files and folders
- Accessible close button
- Keyboard hint (ESC) displayed with the Kbd component
All previously flagged issues have been addressed.
381-402: LGTM! Search and edit buttons are well-integrated.The new action buttons properly:
- Disable the search button when no entries are available (consistent with the "/" keyboard shortcut)
- Provide clear aria-labels and titles
- Use appropriate sizing and styling
- Maintain layout stability with the shrink-0 class
439-439: LGTM! All necessary types are exported.The export includes the new
FileSystemEntrytype, enabling consumers to properly type their entries arrays and onSelectEntry callbacks.
Shironex
added
Enhancement
…ndling in comments - Replaced inline entry mapping with a memoized entryItems component for improved performance. - Clarified keydown event handling comments to enhance understanding of ESC key behavior in relation to modal interactions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/components/ui/path-input.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). (1)
- GitHub Check: e2e
🔇 Additional comments (3)
apps/ui/src/components/ui/path-input.tsx (3)
64-68: LGTM on FileSystemEntry type and memoized rendering.The
FileSystemEntrytype is clean and well-suited for the search functionality. TheentryItemsmemoization is a good optimization for potentially large directory listings, ensuring CommandItem elements aren't recreated on every render.Also applies to: 256-269
306-340: LGTM on search popover implementation.The Command-based search UI is well-structured with proper focus management (
autoFocus), consistent keyboard hint display using theKbdcomponent, and appropriate accessibility attributes. The overlay positioning and max-height constraint provide good UX for the search results.
384-394: Good consistency between button state and keyboard shortcut.The search button's disabled state (
entries.length === 0) correctly matches the "/" keyboard shortcut condition on line 220, preventing inconsistent behavior between mouse and keyboard users.
… does not close when the search is active and the ESC key is pressed
…improved keyboard shortcut handling - Introduced useOSDetection hook to determine the user's operating system. - Updated FileBrowserDialog to utilize the OS detection for displaying the correct keyboard shortcut (⌘ or Ctrl) based on the detected OS.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/ui/src/hooks/use-os-detection.ts (1)
38-47: Consider removing useMemo for simpler implementation.The
useMemowith an empty dependency array doesn't provide meaningful performance benefits here sincedetectOS()is a synchronous pure function. The hook could be simplified:🔎 Optional refactor to remove useMemo
export function useOSDetection(): OSDetectionResult { - return useMemo(() => { - const os = detectOS(); - return { - os, - isMac: os === 'mac', - isWindows: os === 'windows', - isLinux: os === 'linux', - }; - }, []); + const os = detectOS(); + return { + os, + isMac: os === 'mac', + isWindows: os === 'windows', + isLinux: os === 'linux', + }; }This also removes the now-unused
useMemoimport. However, if you expect to add stateful logic or effects in the future, keeping the memoization is fine.apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
258-263: Optional: Remove redundant isDirectory check.Since
entriesonly contains directories (all mapped withisDirectory: true), the conditional check on line 260 is redundant. You could simplify:🔎 Optional refactor
entries={directories.map((dir) => ({ ...dir, isDirectory: true }))} - onSelectEntry={(entry) => { - if (entry.isDirectory) { - handleSelectDirectory(entry); - } - }} + onSelectEntry={handleSelectDirectory}However, the explicit check provides safety if the code evolves to include file entries in the future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/hooks/index.tsapps/ui/src/hooks/use-os-detection.ts
⏰ 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: e2e
🔇 Additional comments (7)
apps/ui/src/hooks/use-os-detection.ts (2)
1-10: LGTM! Clean type definitions.The type definitions are well-structured with appropriate use of readonly properties and literal union types.
12-32: Current code already safely handles bundler polyfill scenarios.The check on line 14 is correct as written. The
typeof process !== 'undefined'guard combined with the truthiness check onprocess.platformensures safe access:
- In Electron:
processis the real Node.js global with aplatformproperty- In browser without polyfill:
typeof process === 'undefined', so the block is skipped- In browser with incomplete polyfill:
process.platformisundefined(falsy), so the block is skipped and the fallback tonavigator.platformis usedThe suggested additional type check (
typeof process.platform === 'string') is defensive but unnecessary sinceprocess.platformis always a non-empty string in Node.js or properly caught as undefined by the existing truthiness check.apps/ui/src/hooks/index.ts (1)
6-6: LGTM! Proper export pattern.The export follows the established pattern in this file and correctly includes both the hook and its associated types.
apps/ui/src/components/dialogs/file-browser-dialog.tsx (4)
72-72: LGTM! Clean hook usage.The hook usage correctly extracts only the needed
isMacproperty for conditional keyboard hint rendering.
238-238: Verify focus removal doesn't impact keyboard navigation accessibility.Adding
focus:outline-noneandfocus-visible:outline-noneremoves the default focus indicator. Ensure that:
- The Dialog component provides its own focus management
- Keyboard users can still identify which element has focus
- This aligns with WCAG 2.4.7 (Focus Visible) guidelines
If custom focus indicators aren't implemented elsewhere, consider removing these classes or implementing alternative focus styling.
378-381: LGTM! Clean keyboard hint implementation.The keyboard hint correctly displays platform-specific modifier keys (⌘ for Mac, Ctrl for others) and uses the universal return symbol (↵). The implementation is clean and follows good UX patterns.
13-13: Imports are valid. TheKbdandKbdGroupcomponents exist inapps/ui/src/components/ui/kbd.tsxand are properly exported.
3 participants
Summary
/to open,ESCto close)Changes
New Components
PathInput Component
/key to activate searchESCkey to close searchentries: List of file system entries to search throughonSelectEntry: Callback when user selects an entry from searchsearchPlaceholder: Custom placeholder for search inputFileBrowserDialog Component
<kbd>element with new Kbd/KbdGroup components for better consistencyfocus:outline-noneto DialogContent for cleaner focus statesUX Improvements
/andESCshortcutsScreenshots
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.