refactor(comments-list): reduce Tooltip Provider context overhead and duplication #25771
Conversation
WalkthroughThe comments list component is being restructured to wrap content in a TooltipProvider for enhanced tooltip functionality. Multiple new UI elements are introduced: conditional author avatar and name display with filter actions, post context information with optional filter buttons, post feature images, dropdown menus providing "View post" and "View member" actions, and Show/Hide controls for published comments. Date display is enhanced with tooltips showing formatted dates. The delete confirmation dialog structure is realigned with the updated wrapper. Existing virtualization mechanics and placeholders are preserved. The component's public signature remains unchanged. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20-25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/posts/src/views/comments/components/comments-list.tsx (1)
163-170: Delete functionality is incomplete – dialog cannot be triggered.The
commentToDeletestate andconfirmDeletefunction are defined, and anAlertDialogexists (lines 373-391), but there is no UI element that callssetCommentToDelete(item)to trigger the delete flow. This results in unreachable code.If delete functionality is planned for a future PR, consider removing this code and adding it when the feature is complete. Otherwise, add a delete button (e.g., in the dropdown menu).
🔎 Example: Add delete option to the dropdown menu
{item.member?.id && ( <DropdownMenuItem asChild> <a href={`#/members/${item.member.id}`}> <LucideIcon.User className="mr-2 size-4" /> View member </a> </DropdownMenuItem> )} + <DropdownMenuItem + className="text-destructive focus:text-destructive" + onClick={() => setCommentToDelete(item)} + > + <LucideIcon.Trash2 className="mr-2 size-4" /> + Delete + </DropdownMenuItem> </DropdownMenuContent>
🧹 Nitpick comments (2)
apps/posts/src/views/comments/components/comments-list.tsx (2)
87-127: Consider debouncing the resize event listener.The
checkIfClampedfunction is called on every resize event without debouncing. With many comment rows visible (virtualized list with ~20 visible items), this could trigger many layout calculations during window resizing.🔎 Proposed fix using a debounced resize handler
+import {forwardRef, useEffect, useRef, useState, useMemo} from 'react'; + +// Add a simple debounce utility or import from lodash +function debounce<T extends (...args: unknown[]) => void>(fn: T, ms: number): T { + let timeoutId: ReturnType<typeof setTimeout>; + return ((...args: unknown[]) => { + clearTimeout(timeoutId); + timeoutId = setTimeout(() => fn(...args), ms); + }) as T; +} function CommentContent({item}: {item: Comment}) { const contentRef = useRef<HTMLDivElement>(null); const [isClamped, setIsClamped] = useState(false); const [isExpanded, setIsExpanded] = useState(false); + const checkIfClamped = useMemo(() => debounce(() => { + if (contentRef.current) { + setIsClamped(contentRef.current.scrollHeight > contentRef.current.clientHeight); + } + }, 100), []); useEffect(() => { - const checkIfClamped = () => { - if (contentRef.current) { - setIsClamped(contentRef.current.scrollHeight > contentRef.current.clientHeight); - } - }; - checkIfClamped(); window.addEventListener('resize', checkIfClamped); return () => window.removeEventListener('resize', checkIfClamped); - }, [item.html]); + }, [item.html, checkIfClamped]);
325-353: Consider hiding dropdown trigger when no actions are available.If both
item.post?.urlanditem.member?.idare falsy, the dropdown menu will render with no items, which is a poor user experience. Consider conditionally rendering the dropdown trigger.🔎 Proposed fix
+ {(item.post?.url || item.member?.id) && ( <DropdownMenu> <DropdownMenuTrigger asChild> <Button className="relative z-10 ml-1" size="sm" variant="ghost" > <LucideIcon.Ellipsis /> </Button> </DropdownMenuTrigger> <DropdownMenuContent align="start"> {item.post?.url && ( <DropdownMenuItem asChild> <a href={item.post.url} rel="noopener noreferrer" target="_blank"> <LucideIcon.ExternalLink className="mr-2 size-4" /> View post </a> </DropdownMenuItem> )} {item.member?.id && ( <DropdownMenuItem asChild> <a href={`#/members/${item.member.id}`}> <LucideIcon.User className="mr-2 size-4" /> View member </a> </DropdownMenuItem> )} </DropdownMenuContent> </DropdownMenu> + )}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/posts/src/views/comments/components/comments-list.tsx(4 hunks)
⏰ 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: Cursor Bugbot
🔇 Additional comments (3)
apps/posts/src/views/comments/components/comments-list.tsx (3)
173-173: Good refactor: TooltipProvider consolidation.Wrapping the entire list with a single
TooltipProviderinstead of per-row instances correctly addresses the PR objective. This reduces React context overhead and enables tooltip coordination features likeskipDelayDurationacross all tooltips within the list.
373-391: AlertDialog structure looks good.The delete confirmation dialog is properly structured with accessible title, description, and action buttons. The red styling for the destructive action is appropriate.
Note: This dialog is currently unreachable (see earlier comment about missing delete trigger).
109-111: Server-side sanitization mitigates XSS risk fromdangerouslySetInnerHTML.The Comment model in the Ghost backend applies restrictive HTML sanitization when comments are saved, allowing only safe tags (
p,br,a,blockquote) and enforcing safe link attributes (target="_blank", rel constraints). Theitem.htmlreceived by this component is already sanitized at the API level, making thedangerouslySetInnerHTMLusage safe. Consider adding a code comment documenting this server-side sanitization assumption for future maintainers.
| {item.member.avatar_image && ( | ||
| <div className='absolute inset-0'><img src={item.member.avatar_image} /></div> | ||
| )} |
There was a problem hiding this comment.
Add empty alt attribute for decorative avatar image.
The avatar image is missing an alt attribute. Since the member name is displayed alongside it, the image is decorative and should have alt="" to be properly ignored by screen readers.
- <div className='absolute inset-0'><img src={item.member.avatar_image} /></div>
+ <div className='absolute inset-0'><img alt="" src={item.member.avatar_image} /></div>📝 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.
| {item.member.avatar_image && ( | |
| <div className='absolute inset-0'><img src={item.member.avatar_image} /></div> | |
| )} | |
| {item.member.avatar_image && ( | |
| <div className='absolute inset-0'><img alt="" src={item.member.avatar_image} /></div> | |
| )} |
🤖 Prompt for AI Agents
In apps/posts/src/views/comments/components/comments-list.tsx around lines 216
to 218, the avatar <img> is missing an alt attribute; since the member name is
shown next to it the image is purely decorative, so add an explicit empty alt
attribute (alt="") to the <img> element to ensure screen readers ignore it; if
your project uses an image component wrapper ensure it receives alt="" or
equivalent aria-hidden/role if required by linting.
There was a problem hiding this comment.
Yes, I already noticed the missed alt prop in terms of accessibility, but the PR changes means for TooltipProvider. The core team can create a follow-up ticket for this minor issue.
There was a problem hiding this comment.
@alamenai, that makes sense — keeping the PR focused on the TooltipProvider refactoring is a good approach.
Would you like me to create a GitHub issue to track the missing alt attribute on the avatar image so the core team can address it in a follow-up?
🧠 Learnings used
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 25271
File: apps/stats/src/views/Stats/Growth/Growth.tsx:199-204
Timestamp: 2025-11-12T22:26:48.725Z
Learning: In apps/stats/src/views/Stats/Growth/Growth.tsx, the loading skeleton for the top posts/pages table intentionally uses colSpan={1} to only span the first (wider) column, creating a visual effect where the skeleton appears only in the main content column (which contains post/page titles with dates).
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 25271
File: apps/stats/src/views/Stats/Growth/components/GrowthSources.tsx:189-199
Timestamp: 2025-11-12T22:26:03.622Z
Learning: In apps/stats/src/views/Stats/Growth/components/GrowthSources.tsx, the loading skeleton for the GrowthSources table intentionally uses colSpan={1} to only span the first (wider) column, creating a visual effect where the skeleton appears only in the source column which contains the main content (source names with icons).
Learnt from: jonatansberg
Repo: TryGhost/Ghost PR: 25725
File: apps/shade/src/components/ui/filters.tsx:1227-1233
Timestamp: 2025-12-16T12:24:41.848Z
Learning: In apps/shade/src/components/ui/filters.tsx, when displaying a single selected option with detail in the filter list (around line 1227), the label and detail are intentionally rendered horizontally as sibling spans rather than stacked vertically. A stacked layout would break the filter list UI alignment. This is different from the dropdown/popover contexts where vertical stacking is used.
1 participant
What
Before the changes
In the
comments-listcomponent, we define theTooltipProvidercomponent inside a loop where we recreate new instance per iteration.Each
TooltipProvidercreates a new Context.With virtualized scrolling showing ~20 visible rows, that's:
4 providers × 20 rows = 80 context provider instances.
TooltipProvidermanages shared state for tooltip behavior:With separate providers, tooltips can't coordinate.
When a user hovers from "Replies" → "Likes" → "Reports", the default delay is
700ms, for 3TooltipProviderinstances, this becomes2100ms.The skipDelayDuration feature (which shows subsequent tooltips immediately) only works within the same provider.
From the internal implementation of
TooltipProvider, each instance manages its own state, and based on the current implementation each comment row creates 4 state objects ( Because we declare 4 Tooltip Provider inside a loop ), that means for 20 comments, this is 80 state objects are created and 80 extra nodes in the tree.After the changes
I extracted the
TooltipProviderto wrap the wholecomment-listcomponent once time ( Though it would be better to lift it up to the app-level provider but I wanted to avoid many files changes).Now, we have only one instance of the
TooltipProvidershared between the comments wether we have only one comment or 1000 comments.This will create only 1 shared context node in the tree per list of comments.
Note
Wraps the entire comments list in a single
TooltipProvider, removing redundant per-item providers to reduce context overhead and unify tooltip behavior.apps/posts/src/views/comments/components/comments-list.tsx)TooltipProvider.TooltipProviderinstances inside row content (timestamp and stats tooltips now share the parent provider).Written by Cursor Bugbot for commit 602becf. This will update automatically on new commits. Configure here.