Conversation
Summary of ChangesHello @webdevcody, 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 file browser dialog by introducing a 'recent folders' feature, which stores and displays previously selected directories for quick access. Alongside this new functionality, the dialog's user interface has been refined with various styling adjustments to create a more compact and intuitive experience. 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
|
![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 redesigns the file browser dialog, making it more compact and adding a 'Recent Folders' feature. The implementation of recent folders using localStorage is a great addition. I've found a few issues to address: a bug in the recent folder removal logic, an accessibility issue with nested buttons in the UI, and a missing dependency in a useCallback hook. Addressing these will improve the robustness and accessibility of the new feature.
| function removeRecentFolder(path: string): string[] { | ||
| if (typeof window === "undefined") return []; | ||
| try { | ||
| const recent = getRecentFolders(); | ||
| const updated = recent.filter((p) => p !== path); | ||
| localStorage.setItem(RECENT_FOLDERS_KEY, JSON.stringify(updated)); | ||
| return updated; | ||
| } catch { | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling in removeRecentFolder can lead to a poor user experience. If localStorage.setItem fails (for example, if storage is full or disabled), the function currently returns an empty array []. This will cause the entire list of recent folders to disappear from the UI when the user only intended to remove a single item. The function should return the updated array even if persisting to localStorage fails, ensuring the in-memory state remains correct for the user's session.
| function removeRecentFolder(path: string): string[] { | |
| if (typeof window === "undefined") return []; | |
| try { | |
| const recent = getRecentFolders(); | |
| const updated = recent.filter((p) => p !== path); | |
| localStorage.setItem(RECENT_FOLDERS_KEY, JSON.stringify(updated)); | |
| return updated; | |
| } catch { | |
| return []; | |
| } | |
| } | |
| function removeRecentFolder(path: string): string[] { | |
| if (typeof window === "undefined") return []; | |
| const recent = getRecentFolders(); | |
| const updated = recent.filter((p) => p !== path); | |
| try { | |
| localStorage.setItem(RECENT_FOLDERS_KEY, JSON.stringify(updated)); | |
| } catch { | |
| // Ignore localStorage errors, but return the updated list for UI consistency. | |
| } | |
| return updated; | |
| } |
| <button | ||
| key={folder} | ||
| onClick={() => handleSelectRecent(folder)} | ||
| className="group flex items-center gap-1 h-6 px-2 text-xs bg-sidebar-accent/20 hover:bg-sidebar-accent/40 rounded border border-sidebar-border transition-colors" | ||
| disabled={loading} | ||
| title={folder} | ||
| > | ||
| <Folder className="w-3 h-3 text-brand-500 shrink-0" /> | ||
| <span className="truncate max-w-[120px]">{getFolderName(folder)}</span> | ||
| <button | ||
| onClick={(e) => handleRemoveRecent(e, folder)} | ||
| className="ml-0.5 opacity-0 group-hover:opacity-100 hover:text-destructive transition-opacity" | ||
| title="Remove from recent" | ||
| > | ||
| <X className="w-3 h-3" /> | ||
| </button> | ||
| </button> |
There was a problem hiding this comment.
Nesting a <button> element inside another <button> is invalid HTML. This can lead to unpredictable behavior with event handling and creates significant accessibility issues for users of screen readers and keyboard navigation.
I recommend restructuring this part of the UI to use a container div with two sibling buttons inside. This will make the HTML valid and accessible while preserving the intended functionality and visual grouping.
<div
key={folder}
className="group flex items-center justify-between h-6 px-2 text-xs bg-sidebar-accent/20 hover:bg-sidebar-accent/40 rounded border border-sidebar-border transition-colors"
title={folder}
>
<button
onClick={() => handleSelectRecent(folder)}
className="flex items-center gap-1 flex-grow truncate"
disabled={loading}
>
<Folder className="w-3 h-3 text-brand-500 shrink-0" />
<span className="truncate max-w-[120px]">{getFolderName(folder)}</span>
</button>
<button
onClick={(e) => handleRemoveRecent(e, folder)}
className="ml-0.5 opacity-0 group-hover:opacity-100 hover:text-destructive transition-opacity"
title="Remove from recent"
>
<X className="w-3 h-3" />
</button>
</div>
| const handleSelectRecent = useCallback((path: string) => { | ||
| browseDirectory(path); | ||
| }, []); |
There was a problem hiding this comment.
The dependency array for this useCallback is missing the browseDirectory function. Because browseDirectory is defined within the component scope, it is recreated on every render. Without including it as a dependency, handleSelectRecent will close over a stale version of the function from the initial render, which can lead to bugs.
To fix this, you should first wrap the browseDirectory function in its own useCallback(..., []) to memoize it, and then add browseDirectory to the dependency array here.
| const handleSelectRecent = useCallback((path: string) => { | |
| browseDirectory(path); | |
| }, []); | |
| const handleSelectRecent = useCallback((path: string) => { | |
| browseDirectory(path); | |
| }, [browseDirectory]); |
- Marked the test for opening a project via file browser as skipped in CI due to its unreliability in headless environments. - This change aims to maintain the stability of the test suite while addressing the underlying issue in future updates.
1 participant
No description provided.