Conversation
ref https://linear.app/ghost/issue/FEA-491/design-truncation-and-display-for-long-comments - Longer comments in the comments UI need better truncation and display treatment. The PR clamps the original comment to show max 2 lines and make it possible to view the full comment.
…ests" This reverts commit 10e1f16.
WalkthroughThe changes add support for displaying post feature images in comments UI. The API type definition adds an optional feature_image field to comments, a serializer includes feature_image in the post response payload, and the comments list component is redesigned with enhanced UI using modular components for improved rendering and interactivity. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/posts/src/views/comments/components/comments-list.tsx`:
- Around line 92-104: The effect that computes clamping in useEffect (the
function using contentRef and setIsClamped) must list the content it depends on
so it updates when comment content changes: add item.html to the dependency
array of that useEffect and ensure the effect recalculates clamping and
re-attaches the resize listener accordingly; also reset isExpanded to false
whenever item.html changes (e.g., inside the same effect or a separate one that
watches item.html) so expansion state doesn’t persist across new content.
🧹 Nitpick comments (2)
apps/posts/src/views/comments/components/comments-list.tsx (2)
200-207: Consider removing or tracking the commented-out avatar image code.The commented-out
AvatarImagecode adds clutter. If Gravatar support is intentionally disabled pending a fallback, consider adding a TODO comment with a tracking reference, or remove the code entirely and re-add it when ready.♻️ Option 1: Add tracking comment
<Avatar className={`size-5 ${item.status === 'hidden' && 'opacity-40'}`}> - {/* {item.member.avatar_image && ( - <AvatarImage alt={item.member.name} src={item.member.avatar_image} /> - )} */} + {/* TODO: Re-enable AvatarImage once Gravatar fallback is implemented. See issue `#XXX` */} <AvatarFallback>♻️ Option 2: Remove commented code
<Avatar className={`size-5 ${item.status === 'hidden' && 'opacity-40'}`}> - {/* {item.member.avatar_image && ( - <AvatarImage alt={item.member.name} src={item.member.avatar_image} /> - )} */} <AvatarFallback>
276-317: Consider wrapping tooltips with a singleTooltipProvider.Multiple
TooltipProviderinstances are created for each tooltip (replies, likes, reports). A singleTooltipProvidercan wrap all tooltips in this section for better performance.♻️ Proposed refactor
- <TooltipProvider> - <Tooltip> + <Tooltip> <TooltipTrigger asChild> <div className={`ml-2 flex items-center gap-1 text-xs ${!item.count?.replies && 'text-muted-foreground/70'}`}> <LucideIcon.Reply size={16} strokeWidth={1.5} /> <span>{formatNumber(item.count?.replies)}</span> </div> </TooltipTrigger> <TooltipContent> Replies </TooltipContent> - </Tooltip> - </TooltipProvider> + </Tooltip>Apply the same pattern to the likes and reports tooltips, and wrap the parent
<div className='ml-4 flex items-center gap-3'>with a single<TooltipProvider>.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/activity-feed.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
apps/admin-x-framework/src/api/comments.tsapps/posts/src/views/comments/components/comments-list.tsxghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js
🧰 Additional context used
🪛 ast-grep (0.40.5)
apps/posts/src/views/comments/components/comments-list.tsx
[warning] 109-109: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
apps/posts/src/views/comments/components/comments-list.tsx
[error] 110-110: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (5)
apps/admin-x-framework/src/api/comments.ts (1)
28-28: LGTM!The optional
feature_imagefield addition aligns with the backend serializer changes and follows the existing type patterns in this interface.ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js (1)
34-40: LGTM!The
feature_imagefield is correctly added topostFields, enabling the frontend to display post feature images in comment rows. The existing_.pickpattern handles optional/missing values gracefully.apps/posts/src/views/comments/components/comments-list.tsx (3)
351-357: LGTM!The feature image is conditionally rendered with a proper alt text fallback and responsive visibility. Consider adding an
onErrorhandler to gracefully hide broken images in the future, but this is acceptable as-is.
135-135:onAddFilterprop is required and properly provided by all callers.The prop is indeed marked as required (no optional marker), but the single usage of
CommentsListin the codebase properly providesonAddFilter={handleAddFilter}, so there is no actual breaking change.
109-113: No XSS risk—HTML is sanitized server-side before storage.The Comment model sanitizes all HTML content in its
onSaving()hook using thesanitize-htmllibrary, allowing only safe tags (p,br,a,blockquote) and attributes. Links are automatically transformed withtarget="_blank"andrel="ugc noopener noreferrer nofollow". Theitem.htmlrendered viadangerouslySetInnerHTMLis therefore safe, as it contains pre-sanitized content that has already been validated and restricted on the backend before being stored and served to the frontend.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| useEffect(() => { | ||
| const checkIfClamped = () => { | ||
| if (contentRef.current) { | ||
| // Check if the content is clamped by comparing scrollHeight with clientHeight | ||
| setIsClamped(contentRef.current.scrollHeight > contentRef.current.clientHeight); | ||
| } | ||
| }; | ||
|
|
||
| checkIfClamped(); | ||
| // Recheck on window resize | ||
| window.addEventListener('resize', checkIfClamped); | ||
| return () => window.removeEventListener('resize', checkIfClamped); | ||
| }, []); |
There was a problem hiding this comment.
Missing dependency in useEffect may cause stale clamping state.
The useEffect has an empty dependency array but should include item.html since the clamping calculation depends on the content. If item.html changes without remounting the component, isClamped won't update.
Additionally, isExpanded should reset when content changes.
🔧 Proposed fix
useEffect(() => {
const checkIfClamped = () => {
if (contentRef.current) {
// Check if the content is clamped by comparing scrollHeight with clientHeight
setIsClamped(contentRef.current.scrollHeight > contentRef.current.clientHeight);
}
};
checkIfClamped();
// Recheck on window resize
window.addEventListener('resize', checkIfClamped);
return () => window.removeEventListener('resize', checkIfClamped);
- }, []);
+ }, [item.html]);Also consider resetting expansion state when content changes:
+ useEffect(() => {
+ setIsExpanded(false);
+ }, [item.html]);📝 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.
| useEffect(() => { | |
| const checkIfClamped = () => { | |
| if (contentRef.current) { | |
| // Check if the content is clamped by comparing scrollHeight with clientHeight | |
| setIsClamped(contentRef.current.scrollHeight > contentRef.current.clientHeight); | |
| } | |
| }; | |
| checkIfClamped(); | |
| // Recheck on window resize | |
| window.addEventListener('resize', checkIfClamped); | |
| return () => window.removeEventListener('resize', checkIfClamped); | |
| }, []); | |
| useEffect(() => { | |
| const checkIfClamped = () => { | |
| if (contentRef.current) { | |
| // Check if the content is clamped by comparing scrollHeight with clientHeight | |
| setIsClamped(contentRef.current.scrollHeight > contentRef.current.clientHeight); | |
| } | |
| }; | |
| checkIfClamped(); | |
| // Recheck on window resize | |
| window.addEventListener('resize', checkIfClamped); | |
| return () => window.removeEventListener('resize', checkIfClamped); | |
| }, [item.html]); |
🤖 Prompt for AI Agents
In `@apps/posts/src/views/comments/components/comments-list.tsx` around lines 92 -
104, The effect that computes clamping in useEffect (the function using
contentRef and setIsClamped) must list the content it depends on so it updates
when comment content changes: add item.html to the dependency array of that
useEffect and ensure the effect recalculates clamping and re-attaches the resize
listener accordingly; also reset isExpanded to false whenever item.html changes
(e.g., inside the same effect or a separate one that watches item.html) so
expansion state doesn’t persist across new content.
3 participants
Benchmark PR from qodo-benchmark#228
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.