feat: implement Thread viewport with radix-ui scroll-area primitives #1566
Conversation
|
@PieterT2000 is attempting to deploy a commit to the assistant-ui Team on Vercel. A member of the Team first needs to authorize it. |
|
LGTM 👍 |
|
📝 Documentation updates detected! You can review documentation updates here The PR implements a significant change to the Thread component's scrollbar functionality, making it a built-in feature using Radix UI's scroll-area primitives. I've made the following documentation updates:
The old Scrollbar.mdx documentation file has been removed as part of the PR since the functionality is now built into the core components. No additional documentation updates are needed since the feature is now handled automatically by the Thread component. Let me summarize the changes for the user. I've updated the documentation to reflect the new built-in scrollbar functionality in the Thread component. The main changes are:
These changes align with the PR's implementation of Thread viewport with Radix UI scroll-area primitives, making the custom scrollbar functionality a built-in feature rather than requiring manual setup. |
WalkthroughThe pull request removes the documentation file for implementing a custom scrollbar and introduces new scroll area components. A new file is added that defines ScrollArea and ScrollBar components using primitives from the @radix-ui/react-scroll-area library. The ThreadRoot and ThreadViewport components are updated to replace previous div implementations with these scroll area primitives, with corresponding updates to type definitions. In addition, a new ScrollBar component is added to another part of the codebase to manage scrollbar styling and functionality. The dependency on @radix-ui/react-scroll-area is updated in multiple package.json files to version 1.2.1, and the registry of components now includes an entry for "scroll-area". No changes were made to declarations of exported or public entities apart from the necessary updates to component references and type definitions. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/registry/components/ui/scroll-area.tsxOops! Something went wrong! :( ESLint: 9.20.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/react/src/primitives/thread/ThreadViewport.tsxOops! Something went wrong! :( ESLint: 9.20.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by apps/registry/src/registry.tsOops! Something went wrong! :( ESLint: 9.20.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/docs/content/docs/ui/shadcn-ui/Scrollbar.mdx(0 hunks)apps/registry/components/ui/scroll-area.tsx(1 hunks)apps/registry/package.json(1 hunks)apps/registry/src/registry.ts(1 hunks)packages/react/package.json(1 hunks)packages/react/src/primitives/thread/ThreadRoot.tsx(1 hunks)packages/react/src/primitives/thread/ThreadViewport.tsx(1 hunks)packages/react/src/ui/base/scroll-area.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/docs/content/docs/ui/shadcn-ui/Scrollbar.mdx
🔇 Additional comments (9)
packages/react/src/primitives/thread/ThreadRoot.tsx (3)
1-4: LGTM! Clean import statements.The imports are well-organized and include all necessary dependencies for the ScrollArea implementation.
7-8: LGTM! Type definitions are properly updated.The Element and Props types are correctly updated to use ScrollAreaPrimitive.Root.
14-24: LGTM! Clean implementation of ScrollArea.The implementation correctly:
- Uses ScrollAreaPrimitive.Root
- Handles className merging
- Includes ScrollBar component
packages/react/src/ui/base/scroll-area.tsx (1)
7-26: LGTM! Well-structured ScrollBar component.The component is well-implemented with:
- Proper type definitions
- Correct ref forwarding
- Default vertical orientation
packages/react/src/primitives/thread/ThreadViewport.tsx (2)
10-15: LGTM! Type definitions are properly updated.The Element and Props types are correctly updated to use ScrollAreaPrimitive.Viewport while preserving the autoScroll property.
28-34: LGTM! Clean implementation of Viewport.The implementation correctly:
- Uses ScrollAreaPrimitive.Viewport
- Preserves autoScroll functionality
- Handles className merging
apps/registry/components/ui/scroll-area.tsx (1)
8-25: LGTM! Well-structured ScrollArea component.The ScrollArea component is well-implemented with:
- Proper type definitions
- Correct ref forwarding
- Appropriate default styling
apps/registry/src/registry.ts (1)
17-17: LGTM! Dependency registration is correct.The addition of "scroll-area" to the thread component's registry dependencies aligns with the PR objective of implementing the Thread viewport using Radix UI scroll-area primitives.
apps/registry/package.json (1)
18-18: 🌐 Web query:What is the latest version of @radix-ui/react-scroll-area?💡 Result:
The latest version of `@radix-ui/react-scroll-area` is **1.2.3**, published on February 5, 2025[1][9]. This information comes directly from both its npm registry listing and Radix UI's official documentation[9], which also provides installation instructions: ```bash npm install @radix-ui/react-scroll-area # or yarn add @radix-ui/react-scroll-area ``` The package has been actively maintained with recent updates, including release candidates for previous versions like 1.2.0-rc.x[7][8]. The current stable release (1.2.3) includes native scroll behavior preservation while enabling custom styling of scrollbars[9]. Citations: - 1: https://www.npmjs.com/package/@radix-ui/react-scroll-area - 2: https://jsr.io/@radix-ui-fork/react-scroll-area - 3: https://github.com/radix-ui/themes/blob/main/packages/radix-ui-themes/src/components/scroll-area.tsx - 4: https://github.com/radix-ui/primitives/blob/main/packages/react/scroll-area/src/ScrollArea.tsx - 5: https://www.jsdelivr.com/package/npm/@radix-ui/react-scroll-area - 6: https://ui.tailus.io/react/components/scroll-area/ - 7: https://www.assistant-ui.com/docs/ui/styled/Scrollbar - 8: https://npm.io/package/@radix-ui/react-scroll-area - 9: https://www.radix-ui.com/primitives/docs/components/scroll-area - 10: https://www.npmjs.com/package/radix-ui/v/1.1.0?activeTab=dependencies
| >(({ className, orientation = "vertical", ...props }, ref) => ( | ||
| <ScrollAreaPrimitive.ScrollAreaScrollbar | ||
| ref={ref} | ||
| orientation={orientation} | ||
| className={classNames( | ||
| "flex touch-none select-none transition-colors", | ||
| orientation === "vertical" && | ||
| "h-full w-2.5 border-l border-l-transparent p-[1px]", | ||
| orientation === "horizontal" && | ||
| "h-2.5 flex-col border-t border-t-transparent p-[1px]", | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| <ScrollAreaPrimitive.ScrollAreaThumb className="bg-border relative flex-1 rounded-full" /> | ||
| </ScrollAreaPrimitive.ScrollAreaScrollbar> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding aria-label for accessibility.
The ScrollBar component could benefit from an accessibility enhancement.
<ScrollAreaPrimitive.ScrollAreaScrollbar
ref={ref}
orientation={orientation}
+ aria-label={`${orientation} scrollbar`}
className={classNames(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| >(({ className, orientation = "vertical", ...props }, ref) => ( | |
| <ScrollAreaPrimitive.ScrollAreaScrollbar | |
| ref={ref} | |
| orientation={orientation} | |
| className={classNames( | |
| "flex touch-none select-none transition-colors", | |
| orientation === "vertical" && | |
| "h-full w-2.5 border-l border-l-transparent p-[1px]", | |
| orientation === "horizontal" && | |
| "h-2.5 flex-col border-t border-t-transparent p-[1px]", | |
| className, | |
| )} | |
| {...props} | |
| > | |
| <ScrollAreaPrimitive.ScrollAreaThumb className="bg-border relative flex-1 rounded-full" /> | |
| </ScrollAreaPrimitive.ScrollAreaScrollbar> | |
| (({ className, orientation = "vertical", ...props }, ref) => ( | |
| <ScrollAreaPrimitive.ScrollAreaScrollbar | |
| ref={ref} | |
| orientation={orientation} | |
| aria-label={`${orientation} scrollbar`} | |
| className={classNames( | |
| "flex touch-none select-none transition-colors", | |
| orientation === "vertical" && | |
| "h-full w-2.5 border-l border-l-transparent p-[1px]", | |
| orientation === "horizontal" && | |
| "h-2.5 flex-col border-t border-t-transparent p-[1px]", | |
| className, | |
| )} | |
| {...props} | |
| > | |
| <ScrollAreaPrimitive.ScrollAreaThumb className="bg-border relative flex-1 rounded-full" /> | |
| </ScrollAreaPrimitive.ScrollAreaScrollbar> | |
| )) |
| const ScrollBar = React.forwardRef< | ||
| React.ComponentRef<typeof ScrollAreaPrimitive.ScrollAreaScrollbar>, | ||
| React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.ScrollAreaScrollbar> | ||
| >(({ className, orientation = "vertical", ...props }, ref) => ( | ||
| <ScrollAreaPrimitive.ScrollAreaScrollbar | ||
| ref={ref} | ||
| orientation={orientation} | ||
| className={cn( | ||
| "flex touch-none select-none transition-colors", | ||
| orientation === "vertical" && | ||
| "h-full w-2.5 border-l border-l-transparent p-[1px]", | ||
| orientation === "horizontal" && | ||
| "h-2.5 flex-col border-t border-t-transparent p-[1px]", | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| <ScrollAreaPrimitive.ScrollAreaThumb className="bg-border relative flex-1 rounded-full" /> | ||
| </ScrollAreaPrimitive.ScrollAreaScrollbar> | ||
| )); | ||
| ScrollBar.displayName = ScrollAreaPrimitive.ScrollAreaScrollbar.displayName; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider consolidating duplicate ScrollBar implementations.
There appears to be a duplicate implementation of the ScrollBar component between this file and packages/react/src/ui/base/scroll-area.tsx. Consider consolidating these implementations to maintain consistency and reduce code duplication.
Consider moving the ScrollBar component to a shared location and importing it in both places.
| @@ -76,6 +76,7 @@ | |||
| "@radix-ui/react-dialog": "^1.1.6", | |||
| "@radix-ui/react-popover": "^1.1.6", | |||
| "@radix-ui/react-primitive": "^2.0.2", | |||
| "@radix-ui/react-scroll-area": "^1.2.1", | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider aligning version constraints across packages.
The dependency is using a caret range (^1.2.1) while the registry package uses a pinned version (1.2.1). Consider using consistent version constraints across packages to prevent potential version mismatches.
- "@radix-ui/react-scroll-area": "^1.2.1",
+ "@radix-ui/react-scroll-area": "1.2.1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@radix-ui/react-scroll-area": "^1.2.1", | |
| "@radix-ui/react-scroll-area": "1.2.1", |
There was a problem hiding this comment.
PR Summary
This PR implements Radix UI scroll area primitives to enhance the Thread viewport's scrolling functionality, addressing auto-scrolling issues from issue #1125.
- Added
@radix-ui/react-scroll-areav1.2.1 in bothpackages/reactandapps/registryfor consistent scrolling behavior - Replaced basic
Primitive.divwithScrollAreaPrimitive.ViewportinThreadViewport.tsxto improve scrolling functionality - Added new
scroll-area.tsxcomponents in both packages and registry for custom scrollbar implementation - Concerning deletion of
Scrollbar.mdxdocumentation without replacement, potentially impacting developer guidance - Modified
registry.tsto include scroll-area as a dependency for the thread component
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
8 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| )} | ||
| {...props} | ||
| > | ||
| <ScrollAreaPrimitive.ScrollAreaThumb className="bg-border relative flex-1 rounded-full" /> |
There was a problem hiding this comment.
style: Consider adding aria-label to ScrollAreaThumb for better accessibility
| {children} | ||
| <ScrollBar /> |
There was a problem hiding this comment.
logic: ScrollBar should be placed after Viewport component, not directly after children. This will cause layout issues.
| {children} | |
| <ScrollBar /> | |
| <ScrollAreaPrimitive.Viewport> | |
| {children} | |
| </ScrollAreaPrimitive.Viewport> | |
| <ScrollBar /> |
|
@trag-bot didn't find any issues in the code! ✅✨ |
Pull request summary
|
|
Thank you so much, this is an amazing contribution - I need to do some testing on this before releasing it - will get back to you around Monday! |
|
Thanks! By the way, there don't seem to be any unit/integration tests yet. Is that a decision due to the library's rapidly evolving nature, or is it still being considered? The library is still <1.0.0, so users should expect things to break, but I would be happy to contribute in that area anyway. |
|
@Yonom any updates on when this will be merged? Or is this already implemented with this: https://www.assistant-ui.com/docs/ui/Scrollbar |
|
@PieterT2000 sorry for the delay! we are looking to merge this soon, can you update and resolve conflicts? thanks for your contribution! |
|
Sure, no problem! |
Addresses #1125
Important
Implement Thread viewport using Radix UI scroll-area primitives, adding new components and updating existing ones.
ScrollAreaandScrollBarcomponents inscroll-area.tsxusing@radix-ui/react-scroll-area.ThreadPrimitiveRootinThreadRoot.tsxto useScrollAreaPrimitive.Rootand includeScrollBar.ThreadPrimitiveViewportinThreadViewport.tsxto useScrollAreaPrimitive.Viewport.@radix-ui/react-scroll-areaversion1.2.1topackage.jsonin bothapps/registryandpackages/react.Scrollbar.mdxdocumentation file.This description was created by
for 448efd6. It will automatically update as commits are pushed.