-
Notifications
You must be signed in to change notification settings - Fork 489
feat(ProjectSelector): add auto-scroll and improved UX for project picker #245
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
feat(ProjectSelector): add auto-scroll and improved UX for project picker #245
Conversation
|
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. WalkthroughThe changes enhance the project picker UI by adding scroll-to-project functionality via a new ref-based scrolling system in the hook, moving click handling from inner content to outer container in project items, and wiring the scroll reference through the project selector component. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
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 user experience of the project picker component. It introduces an auto-scroll feature that centers the currently selected project upon opening, adds a visually distinct and theme-aware scrollbar for improved navigation, and expands the clickable area of project items for easier selection. These changes aim to make project selection more intuitive and efficient, particularly for users managing a large number of projects. 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
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsxapps/ui/src/components/layout/sidebar/components/sortable-project-item.tsxapps/ui/src/components/layout/sidebar/hooks/use-project-picker.tsapps/ui/src/styles/global.css
⏰ 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 (9)
apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts (4)
33-47: LGTM!The
scrollToProjecthelper is correctly implemented withscrollIntoViewandblock: 'nearest'for minimal scroll distance.
77-83: LGTM!Resetting the selection index to 0 when the search query changes is appropriate UX behavior.
85-93: LGTM!This effect correctly ensures the highlighted project stays visible during keyboard navigation.
22-22: LGTM!The
scrollContainerRefis properly typed and will be attached to the scroll container in the consuming component.apps/ui/src/components/layout/sidebar/components/sortable-project-item.tsx (2)
34-34: LGTM!Moving the click handler to the outer container expands the clickable area, improving UX especially on smaller screens. The
stopPropagationon the drag handle correctly prevents accidental selections during drag operations.
47-57: LGTM!The inner content div is now correctly presentational-only, delegating click handling to the parent container.
apps/ui/src/styles/global.css (1)
873-897: LGTM!The scrollbar styling is consistent with existing patterns in this file (e.g.,
scrollbar-styled) and properly uses theme-aware CSS variables.apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsx (2)
67-67: LGTM!The
scrollContainerRefis properly destructured from the hook.
175-178: LGTM!The scroll container is properly configured with the ref attachment and appropriate CSS classes for smooth scrolling and styled scrollbar.
apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts
Outdated
Show resolved
Hide resolved
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 introduces several nice user experience improvements to the project picker, including auto-scrolling to the current project, a visible scrollbar, and larger click targets. The implementation is solid overall. My feedback includes a critical fix for the useProjectPicker hook to prevent stale state and handle all cases correctly, a minor cleanup for a type assertion, and a suggestion to refactor duplicated CSS for scrollbar styling to improve maintainability.
apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts
Outdated
Show resolved
Hide resolved
apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts
Outdated
Show resolved
Hide resolved
|
@illia1f if you can address those comments from coderabbit about the stale closure and see if they are real issues or not, I will merge after |
…nd improve selection logic
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts (1)
62-79: Great fix! Stale closure issue resolved.This effect now correctly includes
filteredProjectsin the dependency array, addressing the critical concern from previous reviews. The logic cleanly separates search and non-search cases.Optional refinement for better precision
Since the effect only uses
currentProject?.id, you could optimize the dependency to be more precise:- }, [isProjectPickerOpen, projectSearchQuery, filteredProjects, currentProject]); + }, [isProjectPickerOpen, projectSearchQuery, filteredProjects, currentProject?.id]);This would prevent unnecessary re-runs when
currentProjectobject reference changes but theidremains the same. However, the current implementation is correct and this is purely an optimization.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsxapps/ui/src/components/layout/sidebar/hooks/use-project-picker.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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (7)
apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts (5)
22-22: LGTM! Ref initialization is correct.The
scrollContainerRefis properly typed asHTMLDivElementand initialized withnull.
49-60: Excellent separation of concerns!This effect cleanly handles the open/close lifecycle by focusing the search input when opening and resetting the query when closing. The use of
requestAnimationFrameensures the DOM is ready before focusing.
81-92: Excellent! All dependencies correctly declared.This effect now properly includes
scrollToProjectandfilteredProjectsin the dependency array, resolving the stale closure concerns from previous reviews. TherequestAnimationFrameensures the DOM is rendered before scrolling.
143-143: LGTM! Ref correctly exposed in public API.The
scrollContainerRefis properly returned for use in consuming components.
33-47: Implementation is correct. ThescrollToProjectfunction is properly memoized and the data-testid attribute is correctly set on project items inSortableProjectItem(line 33 of sortable-project-item.tsx), matching the selector pattern used in the scroll function.apps/ui/src/components/layout/sidebar/components/project-selector-with-options.tsx (2)
67-67: LGTM! Ref correctly destructured from hook.The
scrollContainerRefis properly extracted from theuseProjectPickerhook return value.
175-178: Integration is correct — scrollbar-styled class is properly defined.The
scrollContainerRefis correctly attached to the scrolling container. Thescroll-smoothclass provides smooth scrolling behavior, andscrollbar-styledis defined inapps/ui/src/styles/global.css(lines 499–519) with theme-specific overrides in the theme CSS files. The implementation matches existing usage patterns in other components.
|
@webdevcody Refactored and ready for review ;) |
Summary
Features
Auto-scroll to current project
Visible scrollbar
Improved click targets
Screenshots
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.