refactor: Extract PathInput component from FileBrowserDialog & Improve UI/UX#262
Conversation
…onent - Removed unused state and imports from FileBrowserDialog. - Replaced direct path input with a new PathInput component for improved navigation. - Enhanced state management for path navigation and error handling. - Updated UI elements for better user experience and code clarity.
- Changed the navigation instruction text in FileBrowserDialog to use an arrow symbol for clarity. - Added an ArrowRight icon to the PathInput component's button for improved visual feedback when navigating to a path.
|
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. 📝 WalkthroughWalkthroughThis PR adds Breadcrumb and PathInput UI components and refactors FileBrowserDialog to use PathInput, removing the legacy manual path input, Go button, local path-input state, and related keyboard handling while preserving directory browsing behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant PI as PathInput
participant FBD as FileBrowserDialog
participant API as Browse API
User->>PI: Click breadcrumb / Home / Edit path
alt Click breadcrumb or Home
PI->>FBD: onNavigate(path) / onHome()
else Edit + Enter
PI->>PI: validate/parse input
PI->>FBD: onNavigate(newPath)
end
FBD->>API: browseDirectory(path)
API-->>FBD: Directory contents / error
alt Success
FBD->>FBD: update currentPath & file list
FBD->>PI: update currentPath prop
PI->>PI: re-render breadcrumbs
else Error
FBD->>PI: indicate error, request focus
PI->>User: focus & select input for retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 refactors the file browser dialog by extracting its path navigation functionality into a new, highly reusable 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 is an excellent refactoring that significantly improves the modularity and maintainability of the FileBrowserDialog by extracting the path handling logic into a new PathInput component. The new component is well-designed, with a clean API and good user experience features like click-to-edit and keyboard navigation. The code is clean and easy to follow.
I've found one minor bug related to handling the root path and a small performance optimization opportunity in the new PathInput component. My detailed comments are below. Overall, great work on this pull request!
![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)
apps/ui/src/components/ui/path-input.tsx (3)
22-48: Unix root path returns empty breadcrumbs.When
currentPathis"/"(Unix root),split(/[/\\]/).filter(Boolean)yields an empty array, so no breadcrumbs are displayed. Users at the root directory won't see any path indication.Consider handling the root case explicitly:
🔎 Proposed fix
function parseBreadcrumbs(path: string): BreadcrumbSegment[] { if (!path) return []; const segments = path.split(/[/\\]/).filter(Boolean); const isWindows = segments[0]?.includes(':'); + + // Handle Unix root directory + if (!isWindows && segments.length === 0 && path === '/') { + return [{ name: '/', path: '/', isLast: true }]; + } return segments.map((segment, index) => {
118-130: Consider using the genericFocusEventtype for better type safety.The
FocusEventtype without a generic defaults toFocusEvent<Element>. UsingFocusEvent<HTMLInputElement>would provide more precise typing.🔎 Proposed fix
const handleInputBlur = useCallback( - (e: FocusEvent) => { + (e: FocusEvent<HTMLInputElement>) => { // Check if focus is moving to another element within this component if (containerRef.current?.contains(e.relatedTarget)) { return; }
228-238: Consider avoidinghref="#"to prevent potential scroll artifacts.Using
href="#"can cause a brief scroll-to-top flash in some browsers beforepreventDefaulttakes effect. Since this is purely JS-driven navigation, consider using a button or a non-scrolling href.🔎 Proposed fix
<BreadcrumbLink - href="#" + href="" onClick={(e) => { e.preventDefault(); handleBreadcrumbClick(crumb.path); }}Alternatively, refactor
BreadcrumbLinkto supportasChildwith a button.apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
180-197: Inconsistent use ofuseCallback.
handleGoHomeandhandleNavigateare wrapped inuseCallback, buthandleSelectDirectoryandhandleSelectDriveare regular functions. Consider wrapping them for consistency, especially since they're passed as props to child elements.🔎 Proposed fix
- const handleSelectDirectory = (dir: DirectoryEntry) => { - browseDirectory(dir.path); - }; + const handleSelectDirectory = useCallback( + (dir: DirectoryEntry) => { + browseDirectory(dir.path); + }, + [browseDirectory] + ); const handleGoHome = useCallback(() => { browseDirectory(); }, [browseDirectory]); const handleNavigate = useCallback( (path: string) => { browseDirectory(path); }, [browseDirectory] ); - const handleSelectDrive = (drivePath: string) => { - browseDirectory(drivePath); - }; + const handleSelectDrive = useCallback( + (drivePath: string) => { + browseDirectory(drivePath); + }, + [browseDirectory] + );apps/ui/src/components/ui/breadcrumb.tsx (1)
52-63: Consider accessibility implications ofrole="link"on a non-interactive element.Using
role="link"witharia-disabled="true"on a<span>may confuse screen reader users who expect interactive behavior. Thearia-current="page"alone suffices to indicate the current page without implying interactivity.🔎 Proposed fix
function BreadcrumbPage({ className, ...props }: React.ComponentProps<'span'>) { return ( <span data-slot="breadcrumb-page" - role="link" - aria-disabled="true" aria-current="page" className={cn('text-foreground font-normal', className)} {...props} /> ); }Note: This follows the shadcn/ui pattern, so you may want to keep it for consistency with that ecosystem.
📜 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/breadcrumb.tsxapps/ui/src/components/ui/path-input.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/ui/src/components/ui/breadcrumb.tsx (1)
apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
apps/ui/src/components/ui/path-input.tsx (1)
PathInput(288-288)
⏰ 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 (12)
apps/ui/src/components/ui/path-input.tsx (5)
1-14: LGTM!Imports are well-organized, grouping React hooks, icons, and UI components appropriately.
50-67: LGTM!Props interface is well-documented with JSDoc comments and appropriate optional/required designations.
84-99: LGTM!The sync effect correctly preserves user input during editing, and the focus effect provides good UX by selecting text on error for easy correction.
257-280: LGTM!The input section has proper accessibility attributes (
aria-invalid,aria-label) and appropriate disabled states.
288-289: LGTM!Clean exports of the component, helper function, and types for external consumption.
apps/ui/src/components/dialogs/file-browser-dialog.tsx (3)
1-14: LGTM!Imports are cleaned up appropriately, removing unused imports and adding the new
PathInputcomponent.
247-255: LGTM!The
PathInputintegration is clean. Theerrorstring is correctly converted to a boolean with!!error.
351-354: LGTM!The instruction text update aligns well with the new PathInput UI that displays an arrow (→) button for navigation.
apps/ui/src/components/ui/breadcrumb.tsx (4)
7-9: LGTM!The
Breadcrumbcomponent properly setsaria-label="breadcrumb"for accessibility.
11-22: LGTM!Correctly uses an
<ol>element for semantic list structure with proper styling.
65-92: LGTM!Both
BreadcrumbSeparatorandBreadcrumbEllipsishandle accessibility correctly—separator is properly hidden from assistive technology, and ellipsis includes screen reader fallback text.
94-102: LGTM!All components are properly exported for external use.
- Introduced useMemo for breadcrumb parsing to enhance performance. - Updated breadcrumb rendering to utilize memoized values for improved efficiency.
- Added logic to correctly parse and return the root path for Unix-like systems in the breadcrumb segment function.
Shironex
added
Refactor
3 participants
Summary
Refactored the file browser dialog by extracting the path navigation UI into a reusable
PathInputcomponent, improving code organization and maintainability.Changes
New Component:
PathInputCreated
apps/ui/src/components/ui/path-input.tsxwith the following features:Enterto navigate to pathEscapeto cancel and exit edit modeRefactored
FileBrowserDialogpathInput,isEditing,pathInputRefparseBreadcrumbshelper (moved to PathInput)UI/UX Improvements
Screenshot
Breadcrumb mode (default view):
Edit mode (with "→" button):
Error state (red border):
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.