Redesigned comment moderation list layout #42
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.
WalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 87-120: The effect in CommentContent doesn't re-run when the
comment HTML changes; update the useEffect dependency array to include item.html
so checkIfClamped runs whenever the comment content updates (and keep the
existing resize listener cleanup). Optionally, reset isExpanded (e.g.,
setIsExpanded(false)) when item.html changes to avoid carrying expanded state
between different comments.
🧹 Nitpick comments (2)
apps/posts/src/views/comments/components/comments-list.tsx (2)
200-207: Consider removing or implementing the commented-out AvatarImage code.The commented-out
AvatarImageblock suggests Gravatar support was disabled temporarily. If this is intentional for the current release, consider adding a TODO comment explaining why it was disabled, or remove the dead code entirely to reduce noise.
275-317: Consider consolidating TooltipProvider instances.Multiple
TooltipProviderwrappers can be replaced with a single parent provider wrapping all tooltips, reducing component nesting and improving readability.♻️ Suggested refactor
- <div className='ml-4 flex items-center gap-3'> - <TooltipProvider> - <Tooltip> + <TooltipProvider> + <div className='ml-4 flex items-center gap-3'> + <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> - - <TooltipProvider> <Tooltip> <!-- ... likes tooltip ... --> </Tooltip> - </TooltipProvider> - - <TooltipProvider> <Tooltip> <!-- ... reports tooltip ... --> </Tooltip> - </TooltipProvider> - </div> + </div> + </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
🧬 Code graph analysis (1)
apps/posts/src/views/comments/components/comments-list.tsx (6)
apps/admin-x-framework/src/api/comments.ts (1)
Comment(8-35)ghost/core/core/server/models/comment.js (1)
Comment(28-359)apps/shade/src/components/ui/avatar.tsx (2)
Avatar(48-48)AvatarFallback(48-48)apps/shade/src/components/ui/tooltip.tsx (4)
TooltipProvider(33-33)Tooltip(33-33)TooltipTrigger(33-33)TooltipContent(33-33)apps/comments-ui/src/utils/admin-api.ts (2)
hideComment(59-61)showComment(62-64)apps/shade/src/components/ui/dropdown-menu.tsx (4)
DropdownMenu(189-189)DropdownMenuTrigger(190-190)DropdownMenuContent(191-191)DropdownMenuItem(192-192)
🪛 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 (6)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js (1)
34-40: LGTM!The addition of
feature_imagetopostFieldscorrectly enables the serializer to include post thumbnails in the comment response, aligning with the corresponding TypeScript type update and UI implementation.apps/posts/src/views/comments/components/comments-list.tsx (4)
60-71: LGTM!The
formatDatefunction correctly usesIntl.DateTimeFormatfor locale-aware formatting and the regex cleanly removes the comma between day and year as intended.
73-85: LGTM!Clean, focused component with appropriate styling and icon usage for the expand/collapse toggle.
350-358: LGTM!Good implementation with proper alt text for accessibility, responsive visibility, and consistent image sizing with
object-cover.
263-274: LGTM!Clean conditional rendering for hide/show actions. The lack of confirmation dialog is appropriate since these are easily reversible operations, unlike the delete action which correctly uses an
AlertDialog.apps/admin-x-framework/src/api/comments.ts (1)
23-29: LGTM!The
feature_imageaddition to theposttype correctly aligns with the backend serializer changes and is appropriately marked as optional.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| function CommentContent({item}: {item: Comment}) { | ||
| const contentRef = useRef<HTMLDivElement>(null) | ||
| const [isClamped, setIsClamped] = useState(false) | ||
| const [isExpanded, setIsExpanded] = useState(false) | ||
|
|
||
| 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); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <div className={`mt-1 flex flex-col gap-2`}> | ||
| <div className="flex max-w-[720px] flex-col items-start"> | ||
| <div | ||
| dangerouslySetInnerHTML={{__html: item.html || ''}} | ||
| ref={contentRef} | ||
| className={`prose flex-1 text-base leading-[1.45em] ${isExpanded ? '-mb-1 [&_p]:mb-[0.85em]' : 'line-clamp-2 [&_*]:m-0 [&_*]:inline'} ${item.status === 'hidden' && 'text-muted-foreground'}`} | ||
| /> | ||
| {isClamped && ( | ||
| <ExpandButton expanded={isExpanded} onClick={() => setIsExpanded(!isExpanded)} /> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for HTML sanitization patterns
echo "=== Searching for HTML sanitization keywords ==="
rg -i 'sanitize|purify|domPurify|xss|htmlsafe' --type ts --type tsx --type js -n
echo ""
echo "=== Searching for Comment type definition ==="
fd -e ts -e tsx | xargs grep -l "type Comment\|interface Comment" | head -20
echo ""
echo "=== Checking for comment processing/API logic ==="
fd -e ts -e tsx -path "*/comment*" | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 765
🏁 Script executed:
#!/bin/bash
# Search for where item.html is populated/used
echo "=== Searching for 'item.html' usage ==="
rg "item\.html" --type ts --type tsx -n -B2 -A2
echo ""
echo "=== Searching for Comment interface/type ==="
rg "interface Comment|type Comment" --type ts --type tsx -n -A5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Search for HTML sanitization patterns (without type filter)
echo "=== Searching for HTML sanitization keywords ==="
rg -i 'sanitize|purify|domPurify|xss' -n | head -40
echo ""
echo "=== Searching for Comment type/interface definition ==="
rg "type Comment|interface Comment" -n -A8 | head -60Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 10349
🏁 Script executed:
#!/bin/bash
# Check the specific files that likely contain Comment definition
echo "=== Checking comments API file ==="
cat -n apps/admin-x-framework/src/api/comments.ts | head -100
echo ""
echo "=== Checking comments-list.tsx for imports and item.html usage ==="
cat -n apps/posts/src/views/comments/components/comments-list.tsx | head -50Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 5032
🏁 Script executed:
#!/bin/bash
# Look for server-side comment processing/sanitization in ghost core
echo "=== Searching for comment HTML processing in Ghost core ==="
find ghost/core -type f -name "*.js" | xargs grep -l "comment" | grep -E "(model|service|filter|sanitize)" | head -10
echo ""
echo "=== Check if comments are sanitized in the API response ==="
find ghost/core -type f -name "*.js" | xargs grep -l "html" | xargs grep -l "comment" | head -10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1444
🏁 Script executed:
#!/bin/bash
# Search for comment model/processing in Ghost
echo "=== Searching for comment model/serializer ==="
find ghost/core -type f -name "*comment*" | head -20
echo ""
echo "=== Search for where comment.html gets set ==="
find ghost/core -type f -name "*.js" | xargs grep -n "comment.*html\|html.*comment" | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 4388
🏁 Script executed:
#!/bin/bash
# Check the output mapper for comments - this is where HTML gets prepared for API response
echo "=== Checking comments output mapper ==="
cat -n ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 3691
🏁 Script executed:
#!/bin/bash
# Check the CommentsService where html is being set
echo "=== Checking CommentsService for HTML sanitization ==="
cat -n ghost/core/core/server/services/comments/CommentsService.js | head -300Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 11495
🏁 Script executed:
#!/bin/bash
# Continue searching CommentsService for HTML handling
echo "=== More of CommentsService (lines 300-450) ==="
sed -n '300,450p' ghost/core/core/server/services/comments/CommentsService.js
echo ""
echo "=== Search for sanitize-html usage with comments ==="
rg "sanitize" ghost/core/core/server/services/comments/ -A3 -B3Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 5107
🏁 Script executed:
#!/bin/bash
# Check the Comment model for hooks/lifecycle events
echo "=== Checking Comment model definition ==="
cat -n ghost/core/core/server/models/comment.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 16529
🏁 Script executed:
#!/bin/bash
# Check how the component handles re-renders when item prop changes
echo "=== Full CommentContent component ==="
sed -n '87,120p' apps/posts/src/views/comments/components/comments-list.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1553
Add item.html to the useEffect dependency array to ensure the clamping check re-runs when comment content changes.
The useEffect hook only runs once on component mount due to the empty dependency array. When the item prop changes (e.g., displaying a different comment), React re-renders the component and updates the DOM via dangerouslySetInnerHTML, but the effect doesn't re-run to measure the new content's dimensions. This means the clamping state won't update correctly when switching between comments.
Suggested 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]);📝 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.
| function CommentContent({item}: {item: Comment}) { | |
| const contentRef = useRef<HTMLDivElement>(null) | |
| const [isClamped, setIsClamped] = useState(false) | |
| const [isExpanded, setIsExpanded] = useState(false) | |
| 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); | |
| }, []); | |
| return ( | |
| <div className={`mt-1 flex flex-col gap-2`}> | |
| <div className="flex max-w-[720px] flex-col items-start"> | |
| <div | |
| dangerouslySetInnerHTML={{__html: item.html || ''}} | |
| ref={contentRef} | |
| className={`prose flex-1 text-base leading-[1.45em] ${isExpanded ? '-mb-1 [&_p]:mb-[0.85em]' : 'line-clamp-2 [&_*]:m-0 [&_*]:inline'} ${item.status === 'hidden' && 'text-muted-foreground'}`} | |
| /> | |
| {isClamped && ( | |
| <ExpandButton expanded={isExpanded} onClick={() => setIsExpanded(!isExpanded)} /> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| } | |
| function CommentContent({item}: {item: Comment}) { | |
| const contentRef = useRef<HTMLDivElement>(null) | |
| const [isClamped, setIsClamped] = useState(false) | |
| const [isExpanded, setIsExpanded] = useState(false) | |
| 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]); | |
| return ( | |
| <div className={`mt-1 flex flex-col gap-2`}> | |
| <div className="flex max-w-[720px] flex-col items-start"> | |
| <div | |
| dangerouslySetInnerHTML={{__html: item.html || ''}} | |
| ref={contentRef} | |
| className={`prose flex-1 text-base leading-[1.45em] ${isExpanded ? '-mb-1 [&_p]:mb-[0.85em]' : 'line-clamp-2 [&_*]:m-0 [&_*]:inline'} ${item.status === 'hidden' && 'text-muted-foreground'}`} | |
| /> | |
| {isClamped && ( | |
| <ExpandButton expanded={isExpanded} onClick={() => setIsExpanded(!isExpanded)} /> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| } |
🧰 Tools
🪛 ast-grep (0.40.5)
[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)
[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)
🤖 Prompt for AI Agents
In `@apps/posts/src/views/comments/components/comments-list.tsx` around lines 87 -
120, The effect in CommentContent doesn't re-run when the comment HTML changes;
update the useEffect dependency array to include item.html so checkIfClamped
runs whenever the comment content updates (and keep the existing resize listener
cleanup). Optionally, reset isExpanded (e.g., setIsExpanded(false)) when
item.html changes to avoid carrying expanded state between different comments.
Benchmark PR from agentic-review-benchmarks#4
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.