fix: ThreadList a11y improvements #1221
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes primarily enhance accessibility (a11y) in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ThreadList
participant ThreadListNew
participant ThreadListItem
participant ThreadConfig
User->>ThreadList: Interacts with thread
ThreadList->>ThreadListNew: Render new thread button
ThreadListNew->>ThreadConfig: Fetch config
ThreadConfig-->>ThreadListNew: Return config
ThreadListNew-->>User: Display button with aria-current
User->>ThreadListItem: Selects thread item
ThreadListItem->>ThreadListItem: Update active state
Poem
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 (1.23.1)
packages/react/src/primitives/threadList/ThreadListNew.tsxOops! Something went wrong! :( ESLint: 9.15.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 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (2)
packages/react/src/ui/index.ts (1)
Line range hint
26-28: Address the TODO comment regarding name collision.The TODO comment indicates a name collision between
AttachmentUIandAttachment. This should be resolved to prevent potential confusion.Would you like me to help propose a new naming convention or open an issue to track this?
packages/react/src/ui/thread-list.tsx (1)
Line range hint
58-65: Consider adding type constraints for accessibility requirementsTo ensure custom components maintain accessibility features, consider adding type constraints that enforce required accessibility props.
+type AccessibleThreadListItemProps = { + 'aria-current'?: 'page' | boolean; + role?: string; + tabIndex?: number; +}; + namespace ThreadListItems { export type Props = { components?: Partial<{ - ThreadListItem: ThreadListPrimitive.Items.Props["components"]["ThreadListItem"]; + ThreadListItem: React.ComponentType<AccessibleThreadListItemProps & ThreadListPrimitive.Items.Props["components"]["ThreadListItem"]>; }>; }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
.changeset/green-actors-warn.md(1 hunks)packages/react/src/primitives/threadList/ThreadListNew.tsx(1 hunks)packages/react/src/primitives/threadListItem/ThreadListItemRoot.tsx(1 hunks)packages/react/src/styles/tailwindcss/thread.css(1 hunks)packages/react/src/ui/index.ts(2 hunks)packages/react/src/ui/thread-config.tsx(1 hunks)packages/react/src/ui/thread-list-item.tsx(2 hunks)packages/react/src/ui/thread-list.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/react/src/ui/thread-list.tsx (1)
Learnt from: Yonom
PR: Yonom/assistant-ui#1200
File: packages/react/src/ui/thread-config.tsx:195-202
Timestamp: 2024-11-20T09:29:04.049Z
Learning: In `packages/react/src/ui/thread-config.tsx`, when reviewing the `ThreadConfigProvider` component written in TypeScript, avoid suggesting additional null checks on variables like `outerConfig` that have non-null types, as TypeScript's compiler enforces non-nullability.
🪛 Markdownlint (0.35.0)
.changeset/green-actors-warn.md
5-5: null
First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (10)
.changeset/green-actors-warn.md (1)
1-3: LGTM on the patch version bump!
The use of a patch version is appropriate for accessibility improvements as they are backward-compatible bug fixes.
packages/react/src/primitives/threadListItem/ThreadListItemRoot.tsx (1)
21-24: LGTM on the accessibility enhancement!
The addition of the aria-current attribute alongside data-active is a valuable improvement for accessibility. This helps screen readers announce the current/active thread item to users, making the navigation experience more inclusive.
Let's verify the accessibility implementation across related components:
✅ Verification successful
Let me check the content of these files to ensure the aria-current implementation is consistent.
Accessibility implementation is consistent and well-implemented
The aria-current attribute is consistently implemented alongside data-active in both thread-related components (ThreadListItemRoot and ThreadListNew). Both use the same conditional pattern {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}, and there are no conflicting ARIA attributes present. The implementation correctly indicates the current/active state for screen reader users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent aria-current usage across thread-related components
# and verify there are no conflicting aria attributes
# Search for aria-current usage in thread-related components
rg -l "aria-current" packages/react/src/
# Check for potential conflicting aria attributes in the same components
ast-grep --pattern 'aria-selected="$_"' packages/react/src/
ast-grep --pattern 'aria-pressed="$_"' packages/react/src/
Length of output: 290
Script:
#!/bin/bash
# Check the implementation details of aria-current in both files
rg -A 5 -B 5 "aria-current" packages/react/src/primitives/threadListItem/ThreadListItemRoot.tsx packages/react/src/primitives/threadList/ThreadListNew.tsx
Length of output: 2114
packages/react/src/primitives/threadList/ThreadListNew.tsx (3)
Line range hint 11-15: LGTM! Clean hook implementation.
The hook follows React best practices with a clear single responsibility and proper context usage.
Line range hint 17-20: LGTM! Well-structured type definitions.
The TypeScript types are properly defined and maintain type safety through appropriate use of generics.
34-34: LGTM! Good accessibility implementation.
The addition of aria-current properly indicates the active state for assistive technologies.
packages/react/src/ui/thread-list.tsx (1)
39-39: Verify props spreading doesn't override accessibility attributes
The props spreading is correctly placed to allow overrides, but ensure that important accessibility attributes (like aria-* and role) are not accidentally overridden by the spread props.
✅ Verification successful
From analyzing the code, I can now generate the final response:
Props spreading in ThreadListNew is safe for accessibility
The component implementation shows that:
- ThreadListNew is a simple wrapper around the base Button component
- It only adds basic props (className, variant) before spreading
- The Button component itself spreads props after its own props, maintaining proper override behavior
- No critical accessibility attributes are being set that could be accidentally overridden
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of ThreadListNew where aria attributes might be overridden
ast-grep --pattern 'ThreadListNew $$$aria-$$$'
Length of output: 48
Script:
#!/bin/bash
# Let's first check the implementation of ThreadListNew to understand its props and accessibility attributes
ast-grep --pattern 'const ThreadListNew = $$$'
# Also check for any aria attributes in the component
rg -n "aria-" "packages/react/src/ui/thread-list.tsx"
# And check how ThreadListNew is being used across the codebase
rg -n "ThreadListNew" -A 2
Length of output: 3303
Script:
#!/bin/bash
# Let's check the Button component implementation since ThreadListNew uses it
ast-grep --pattern 'const Button = $$$'
# Also check if Button has any default aria attributes
rg -n "aria-" -A 2 -B 2 "packages/react/src/ui/button.tsx"
Length of output: 8764
packages/react/src/ui/thread-list-item.tsx (2)
Line range hint 1-80: Overall accessibility improvements look good!
The changes successfully improve accessibility by:
- Using semantic HTML structure
- Implementing tooltips for better screen reader support
- Using proper component composition with asChild pattern
- Making props more flexible with Partial types
59-80: Verify tooltip accessibility and consider adding fallback ARIA attributes.
The changes improve accessibility by using TooltipIconButton, but consider:
- Adding an explicit
aria-labelas fallback when the tooltip is not visible - Verifying that the tooltip text is screen-reader friendly
Let's verify the tooltip implementation across the codebase:
✅ Verification successful
TooltipIconButton already implements proper accessibility features
The TooltipIconButton component already implements proper accessibility features:
- It includes a visually hidden text using
sr-onlyclass that announces the tooltip content to screen readers - The tooltip text is rendered in both the visually hidden span and the tooltip content
- The component uses semantic button element with proper ARIA attributes inherited from the Button component
The original review comment's concerns have been addressed by the existing implementation, making additional aria-label unnecessary as it would be redundant with the existing screen reader text.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check TooltipIconButton implementation for accessibility attributes
ast-grep --pattern 'TooltipIconButton {
$$$
aria-label={$_}
$$$
}'
# Check other archive button implementations for consistency
rg -A 2 'aria-label.*archive'
Length of output: 106
Script:
#!/bin/bash
# Let's check the TooltipIconButton component implementation
fd -e tsx -e ts "tooltip-icon-button" --exec cat {}
# Check how TooltipIconButton is used across the codebase
rg -l "TooltipIconButton"
# Check if there are any accessibility patterns in similar components
rg -A 3 "aria-label|role.*button" --type typescript --type tsx
# Look for other archive-related components
rg -A 3 "Archive.*Button|Button.*archive" --type typescript --type tsx
Length of output: 4650
packages/react/src/ui/thread-config.tsx (1)
77-77: LGTM! Good accessibility improvement.
Converting from label to tooltip aligns with accessibility best practices for interactive elements. This change provides better screen reader support and maintains consistency with other UI elements in the codebase.
Let's verify that all consumers of this config have been updated:
✅ Verification successful
Change from label to tooltip is properly implemented
The verification shows that:
- No remaining usages of
archive.labelexist in the codebase - The new
archive.tooltipis correctly used inthread-list-item.tsx - The
StringsConfigtype is properly exported and used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usages of archive.label that need to be updated
rg "archive.*label" --type typescript --type tsx
# Search for places where the new tooltip structure is used
rg "archive.*tooltip" --type typescript --type tsx
Length of output: 173
Script:
#!/bin/bash
# Search for any remaining usages of archive.label
rg "archive.*label"
# Search for places where the new tooltip structure is used
rg "archive.*tooltip"
# Let's also look for the StringsConfig type usage to understand the impact
rg "StringsConfig"
Length of output: 385
packages/react/src/styles/tailwindcss/thread.css (1)
230-230: Great accessibility improvements!
The added focus styles enhance keyboard navigation by:
- Using
:focus-visiblefor keyboard-only focus indication - Maintaining visual consistency between hover and focus states
- Providing clear focus indication with a ring outline
| "@assistant-ui/react": patch | ||
| --- | ||
|
|
||
| fix: ThreadList a11y improvements |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding more details to the commit message.
While the message follows the conventional commit format, it would be helpful to specify which accessibility improvements were made (e.g., "fix: Add aria-current and focus styles to ThreadList").
-fix: ThreadList a11y improvements
+fix: Add aria-current attributes and focus styles to ThreadList for improved accessibility📝 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.
| fix: ThreadList a11y improvements | |
| fix: Add aria-current attributes and focus styles to ThreadList for improved accessibility |
🧰 Tools
🪛 Markdownlint (0.35.0)
5-5: null
First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
| @@ -19,7 +19,7 @@ export const ThreadListItemPrimitiveRoot = forwardRef< | |||
|
|
|||
| return ( | |||
| <Primitive.div | |||
| {...(isMain ? { "data-active": "true" } : null)} | |||
| {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust aria-current value for better accessibility
While adding aria-current is a great accessibility improvement, its value should be more specific than "true". The ARIA specification recommends using values like "page", "step", "location", "date", "time", or "false" depending on the type of navigation.
For a thread list item, "page" would be more appropriate as it indicates the current page or screen in a set.
- {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
+ {...(isMain ? { "data-active": "true", "aria-current": "page" } : null)}📝 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.
| {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)} | |
| {...(isMain ? { "data-active": "true", "aria-current": "page" } : null)} |
| export { default as ContentPart } from "./content-part"; | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Accessibility implementation is present but could be enhanced
The components show some accessibility considerations:
- TooltipIconButton includes screen reader text (
aui-sr-only) and proper tooltip implementation - UserActionBar uses semantic button elements and tooltips for better UX
- ContentPart is a simple text wrapper with proper semantic markup (
<p>element)
However, some improvements could be made:
- Add proper ARIA roles and attributes to the action bar items
- Ensure keyboard focus management for the edit functionality
- Consider adding aria-live regions for dynamic content updates
🔗 Analysis chain
Verify accessibility implementation in new components.
Since this PR focuses on a11y improvements, please ensure that the newly added components (ContentPart and UserActionBar) follow accessibility best practices:
- Proper ARIA attributes
- Keyboard navigation support
- Screen reader compatibility
Let's check the accessibility implementation:
Also applies to: 41-41
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for accessibility-related code in the new components
# Expected: Find ARIA attributes, role declarations, and keyboard event handlers
# Search for accessibility-related patterns in ContentPart
echo "Checking ContentPart for accessibility implementations:"
rg -i "aria-|role=|onKey|tabIndex" packages/react/src/ui/content-part
# Search for accessibility-related patterns in UserActionBar
echo "Checking UserActionBar for accessibility implementations:"
rg -i "aria-|role=|onKey|tabIndex" packages/react/src/ui/user-action-bar
Length of output: 545
Script:
#!/bin/bash
# Let's first find the correct paths for these components
echo "Finding ContentPart component:"
fd -e tsx -e ts -e js -e jsx "content-part" packages/
echo "Finding UserActionBar component:"
fd -e tsx -e ts -e jsx -e js "user-action-bar" packages/
# After finding the paths, let's check for accessibility patterns
echo "Checking for accessibility implementations:"
fd -e tsx -e ts -e jsx -e js "(content-part|user-action-bar)" packages/ --exec rg -i "aria-|role=|onKey|tabIndex|focus|keyboard|screen.?reader|accessibility" {}
Length of output: 680
Script:
#!/bin/bash
# Let's examine the content of these components directly
echo "Content of ContentPart component:"
cat packages/react/src/ui/content-part.tsx
echo -e "\nContent of UserActionBar component:"
cat packages/react/src/ui/user-action-bar.tsx
Length of output: 2747
Script:
#!/bin/bash
# Let's check the primitives these components are based on
echo "Checking ContentPartPrimitive implementation:"
cat packages/react/src/primitives/content-part.tsx
echo -e "\nChecking ActionBarPrimitive implementation:"
cat packages/react/src/primitives/action-bar.tsx
echo -e "\nChecking TooltipIconButton implementation:"
cat packages/react/src/ui/base/tooltip-icon-button.tsx
Length of output: 1476
| @@ -31,7 +31,7 @@ export const ThreadListPrimitiveNew = forwardRef< | |||
| return ( | |||
| <Primitive.button | |||
| type="button" | |||
| {...(isMain ? { "data-active": "true" } : null)} | |||
| {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)} | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a more specific aria-current value.
While aria-current="true" is valid, consider using a more semantic value like "page" or "location" to better describe the type of current item. Also, the data-active attribute might be redundant since aria-current already indicates the active state.
- {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
+ {...(isMain ? { "aria-current": "page" } : null)}📝 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.
| {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)} | |
| {...(isMain ? { "aria-current": "page" } : null)} |
| const config = useThreadConfig(); | ||
| const label = config.strings?.threadList?.new?.label ?? "New Thread"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding aria-label for better accessibility
While the label extraction logic is good, consider adding an explicit aria-label to the Button component to ensure screen readers announce it correctly, especially if the label contains any special characters or formatting.
<Button
ref={ref}
className="aui-thread-list-new"
variant="ghost"
+ aria-label={label}
{...props}
>Committable suggestion skipped: line range outside the PR's diff.
| namespace ThreadListItems { | ||
| export type Props = { | ||
| /** | ||
| * Optional custom components to override default thread list items | ||
| */ | ||
| components?: Partial<ThreadListPrimitive.Items.Props["components"]>; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance JSDoc documentation with accessibility requirements
The components prop documentation could be more detailed about accessibility requirements that custom components should meet.
/**
* Optional custom components to override default thread list items
+ * @remarks
+ * Custom components should maintain accessibility features:
+ * - Proper heading structure
+ * - Keyboard navigation support
+ * - ARIA attributes (aria-current, aria-label)
+ * - Focus management
*/📝 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.
| namespace ThreadListItems { | |
| export type Props = { | |
| /** | |
| * Optional custom components to override default thread list items | |
| */ | |
| components?: Partial<ThreadListPrimitive.Items.Props["components"]>; | |
| }; | |
| } | |
| namespace ThreadListItems { | |
| export type Props = { | |
| /** | |
| * Optional custom components to override default thread list items | |
| * @remarks | |
| * Custom components should maintain accessibility features: | |
| * - Proper heading structure | |
| * - Keyboard navigation support | |
| * - ARIA attributes (aria-current, aria-label) | |
| * - Focus management | |
| */ | |
| components?: Partial<ThreadListPrimitive.Items.Props["components"]>; | |
| }; | |
| } |
| const config = useThreadConfig(); | ||
| const fallback = | ||
| config.strings?.threadList?.item?.title?.fallback ?? "New Chat"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding ARIA attributes for better accessibility.
While the config and fallback handling is good, consider enhancing accessibility by:
- Adding an appropriate
aria-labeloraria-labelledbyattribute to the paragraph element. - Consider moving the "New Chat" fallback string to a localization constant.
Example enhancement:
return (
<p
ref={ref}
className={classNames("aui-thread-list-item-title", className)}
+ aria-label={props['aria-label'] ?? fallback}
{...props}
>
<ThreadListItemPrimitive.Title fallback={fallback} />
</p>
);Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
ThreadListcomponent, including improved focus visibility and active thread indication.ContentPartadded to the UI module, replacing several previous exports.Bug Fixes
Documentation
Style