fix(sidebar): add title tooltip to channel name in sidebar#38949
fix(sidebar): add title tooltip to channel name in sidebar#38949dodaa08 wants to merge 1 commit intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThree sidebar item components (Condensed, Extended, Medium) were updated to conditionally forward the title prop to SidebarV2Item—passing it only when it is a string; otherwise undefined. This ensures non-string titles are not propagated as strings to the underlying component. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Hi there, thanks for the contribution! 🚀 💯 Closing this PR because it's duplicated now and it has been fixed here: #38320 Questions? Help needed? Feature Requests?
|
dougfabris
added
the
invalid
|
Got it @dougfabris , I thought the pr wasn't maintained for long time, but sorry about that. |
|
It's just because we don't have time to handle all these contributions so quick. But you did the exact same change and raised a new PR. You could save us time in situations like this :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/meteor/client/sidebar/Item/Condensed.tsx (1)
27-32: Sametitle-before-{...props}ordering andSidebarV2Itemforwarding concerns apply here.See the comments on
Medium.tsx— the prop ordering inconsistency and the need to verifySidebarV2Itemrenderstitleas a DOM attribute apply equally to this component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/sidebar/Item/Condensed.tsx` around lines 27 - 32, The title prop is being passed before the props spread which can be overridden and raises forwarding concerns; in Condensed.tsx adjust the JSX so that either (A) you spread {...props} first and then explicitly pass title={typeof title === 'string' ? title : undefined} after the spread, or (B) delete/omit title from props before spreading and then pass the explicit title prop, and also verify that SidebarV2Item forwards the title to the underlying DOM element (or change it to another prop name) to avoid leaking non-DOM props; update the element that renders SidebarV2Item usage in Condensed.tsx accordingly while keeping handleFocus and handlePointerEnter unchanged.apps/meteor/client/sidebar/Item/Extended.tsx (1)
58-65: Sametitle-before-{...props}ordering andSidebarV2Itemforwarding concerns apply here.See the comments on
Medium.tsx. The explicit forwarding ofhrefandselected(also destructured from the function signature) is correct — without listing them explicitly here they would be silently dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/sidebar/Item/Extended.tsx` around lines 58 - 65, The JSX currently spreads {...props} after the explicit title, which lets incoming props override the explicit values; move the {...props} spread before the explicit props so the explicit title/href/selected take precedence and keep the explicit href and selected (they are destructured and must be forwarded explicitly to avoid being dropped); update the SidebarV2Item element so {...props} appears first, then title={typeof title === 'string' ? title : undefined}, href={href}, selected={selected}, and the event handlers (handleFocus, handlePointerEnter).
🧹 Nitpick comments (2)
apps/meteor/client/sidebar/Item/Medium.tsx (2)
26-31:titleis set before{...props}— inconsistent with the defensive ordering used for event handlers.
onFocusandonPointerEnterare placed after{...props}so they cannot be overridden by a caller. Sincetitleis also destructured from the function signature (and therefore absent from...props), this is currently not a bug. However, placingtitlebefore the spread is inconsistent with the pattern and becomes a latent hazard if the destructuring is ever refactored.♻️ Suggested ordering
<SidebarV2Item - title={typeof title === 'string' ? title : undefined} {...props} + title={typeof title === 'string' ? title : undefined} onFocus={handleFocus} onPointerEnter={handlePointerEnter} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/sidebar/Item/Medium.tsx` around lines 26 - 31, The JSX prop ordering is inconsistent: move title after the spread to match the defensive pattern used for event handlers so callers can override it; specifically, in the SidebarV2Item element adjust props so {...props} comes before title (and keep onFocus={handleFocus} and onPointerEnter={handlePointerEnter} after {...props}) — update the SidebarV2Item usage where title, {...props}, handleFocus, and handlePointerEnter are passed.
26-31: Accessibility gap: nativetitleattribute is not accessible to keyboard or touch users.The HTML
titletooltip is browser-dependent, not shown on keyboard focus, not reliably announced by screen readers, and invisible on touch screens. If the project has accessibility requirements, consider a proper tooltip component (e.g., the FuselageTooltip) that surfaces on both hover and focus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/sidebar/Item/Medium.tsx` around lines 26 - 31, The current usage passes a native title to SidebarV2Item which relies on the browser tooltip (not keyboard/touch/screen-reader friendly); replace the native title with an accessible tooltip component (e.g., Fuselage Tooltip) or at minimum set an explicit accessible label (aria-label/aria-describedby) and render a Tooltip that appears on both hover and focus. Locate the SidebarV2Item usage in Medium.tsx (props title, handleFocus, handlePointerEnter) and: remove the native title string prop, wrap the SidebarV2Item (or its inner label) with the Tooltip component so the tooltip is triggered on pointer enter and focus, and ensure the element exposes aria-label or aria-describedby referencing the tooltip content so keyboard and screen-reader users get the same info.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/client/sidebar/Item/Condensed.tsx`:
- Around line 27-32: The title prop is being passed before the props spread
which can be overridden and raises forwarding concerns; in Condensed.tsx adjust
the JSX so that either (A) you spread {...props} first and then explicitly pass
title={typeof title === 'string' ? title : undefined} after the spread, or (B)
delete/omit title from props before spreading and then pass the explicit title
prop, and also verify that SidebarV2Item forwards the title to the underlying
DOM element (or change it to another prop name) to avoid leaking non-DOM props;
update the element that renders SidebarV2Item usage in Condensed.tsx accordingly
while keeping handleFocus and handlePointerEnter unchanged.
In `@apps/meteor/client/sidebar/Item/Extended.tsx`:
- Around line 58-65: The JSX currently spreads {...props} after the explicit
title, which lets incoming props override the explicit values; move the
{...props} spread before the explicit props so the explicit title/href/selected
take precedence and keep the explicit href and selected (they are destructured
and must be forwarded explicitly to avoid being dropped); update the
SidebarV2Item element so {...props} appears first, then title={typeof title ===
'string' ? title : undefined}, href={href}, selected={selected}, and the event
handlers (handleFocus, handlePointerEnter).
---
Nitpick comments:
In `@apps/meteor/client/sidebar/Item/Medium.tsx`:
- Around line 26-31: The JSX prop ordering is inconsistent: move title after the
spread to match the defensive pattern used for event handlers so callers can
override it; specifically, in the SidebarV2Item element adjust props so
{...props} comes before title (and keep onFocus={handleFocus} and
onPointerEnter={handlePointerEnter} after {...props}) — update the SidebarV2Item
usage where title, {...props}, handleFocus, and handlePointerEnter are passed.
- Around line 26-31: The current usage passes a native title to SidebarV2Item
which relies on the browser tooltip (not keyboard/touch/screen-reader friendly);
replace the native title with an accessible tooltip component (e.g., Fuselage
Tooltip) or at minimum set an explicit accessible label
(aria-label/aria-describedby) and render a Tooltip that appears on both hover
and focus. Locate the SidebarV2Item usage in Medium.tsx (props title,
handleFocus, handlePointerEnter) and: remove the native title string prop, wrap
the SidebarV2Item (or its inner label) with the Tooltip component so the tooltip
is triggered on pointer enter and focus, and ensure the element exposes
aria-label or aria-describedby referencing the tooltip content so keyboard and
screen-reader users get the same info.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/sidebar/Item/Condensed.tsxapps/meteor/client/sidebar/Item/Extended.tsxapps/meteor/client/sidebar/Item/Medium.tsx
📜 Review details
⏰ 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: ⚙️ Variables Setup
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/sidebar/Item/Medium.tsxapps/meteor/client/sidebar/Item/Extended.tsxapps/meteor/client/sidebar/Item/Condensed.tsx
🔇 Additional comments (1)
apps/meteor/client/sidebar/Item/Medium.tsx (1)
26-31: Verify thatSidebarV2Itemforwards thetitleprop to the underlying DOM element.The core concern—whether
titleis rendered as an HTML attribute on the DOM node for tooltips—requires inspecting theSidebarV2Itemcomponent definition in@rocket.chat/fuselage. While the prop is accepted by the component (confirmed by consistent usage across Medium.tsx, Extended.tsx, and Condensed.tsx), the implementation details are not accessible in this environment. Check the.d.tsor source code of@rocket.chat/fuselage(version in your package.json) to confirm thattitleis forwarded to the root element and not omitted or used differently.
Makes sense, thanks for the response. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38949 +/- ##
===========================================
+ Coverage 70.56% 70.61% +0.05%
===========================================
Files 3189 3189
Lines 112702 112720 +18
Branches 20429 20446 +17
===========================================
+ Hits 79526 79598 +72
+ Misses 31115 31070 -45
+ Partials 2061 2052 -9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
2 participants
Proposed changes (including videos or screenshots)
The PR adds
titleattribute to the root sidebar item component in the [Extended] [Medium] [Condensed]This ensures that the full channel name is accessible via a native browser tooltip when hovering over any part of the sidebar item, addressing concerns about truncated channel names.
Screencast.From.2026-02-24.01-01-03.mp4
Steps to test or reproduce
Further comments
This fix was applied specifically to the Rocket.Chat sidebar templates to ensure the tooltip is available on the entire row, now on hover every channel will show it in sidebar, Let me know if any feedbacks on it, thank you.
Summary by CodeRabbit