Improved comments threading UI#27962
Merged
Merged
Conversation
no issue Thread connector styling needs sibling position data from the decorated reply graph so recursive rendering does not pass presentation state through every layer.
no issue The reply connector styling is a presentation concern, so naming it as a layout variant keeps it separate from comment parentage and graph structure.
no issue The new connector treatment belongs with the commentsThreads experiment, so flat replies keep their existing layout when the labs flag is disabled.
Contributor
WalkthroughThis PR refactors the threaded comment UI system by introducing sibling position metadata to the thread graph and propagating layout control props throughout the component hierarchy. The thread graph builder now enriches Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/comments-ui/src/components/content/replies.tsx`:
- Around line 43-49: The replies map rendering passes props to CommentComponent
but omits the useThreading prop, causing nested replies to default to
non-threaded layout; update the JSX where CommentComponent is instantiated
(inside the visibleReplies iteration rendering reply) to pass
useThreading={useThreading} along with existing props (key={reply.id},
comment={reply}, isLastSibling={idx === visibleReplies.length - 1},
layoutVariant={useThreading ? 'reply' : 'root'}, parent={comment}) so each
rendered CommentComponent receives the same threading flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d4cfccc-a603-447c-a96d-a15eaeca2129
📒 Files selected for processing (8)
apps/comments-ui/src/components/content/comment.tsxapps/comments-ui/src/components/content/forms/form.tsxapps/comments-ui/src/components/content/forms/reply-form.tsxapps/comments-ui/src/components/content/replies.tsxapps/comments-ui/src/components/content/reply-tree.tsxapps/comments-ui/src/utils/thread-graph.tsapps/comments-ui/test/unit/components/content/comment.test.jsxapps/comments-ui/test/unit/utils/thread-graph.test.ts
no issue Permalink scroll targets are preserved for context, but opening a reply form can reload comments and should not re-trigger the initial scroll or highlight.
no issue Nested replies need the active threading mode so their own reply forms and children keep the threaded layout instead of falling back to flat reply behavior.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/comments-ui/src/components/content/hooks/use-comment-navigation.ts (1)
236-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid marking permalink as “already scrolled” before iframe scroll actually runs.
Line 267 sets
lastScrolledCommentIdbefore the deferred iframe resize callback fires. If a rerender happens first, effect cleanup cancels that pending callback, and the guard then suppresses retries—so the target may never scroll.💡 Suggested fix
const iframe = findContainingIframe(doc); if (iframe) { - lastScrolledCommentId.current = commentIdToScrollTo; return onIframeResize(iframe, () => { - scrollToComment(element, commentIdToScrollTo, {preserveScrollTarget: commentIdToScrollTo === commentIdFromHash}); + const latestElement = containerRef.current?.ownerDocument.getElementById(commentIdToScrollTo); + if (!latestElement) { + return; + } + + lastScrolledCommentId.current = commentIdToScrollTo; + scrollToComment(latestElement, commentIdToScrollTo, {preserveScrollTarget: commentIdToScrollTo === commentIdFromHash}); }); } lastScrolledCommentId.current = commentIdToScrollTo; scrollToComment(element, commentIdToScrollTo, {preserveScrollTarget: commentIdToScrollTo === commentIdFromHash});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/comments-ui/src/components/content/hooks/use-comment-navigation.ts` around lines 236 - 274, The bug is that lastScrolledCommentId.current is set before the deferred iframe resize callback runs, which lets a rerender cancel the pending callback and incorrectly suppress future scrolls; to fix, don’t assign lastScrolledCommentId.current before calling onIframeResize — instead move the assignment into the onIframeResize callback (after scrollToComment runs) so lastScrolledCommentId is only updated when the actual iframe-scroll completes; update the logic around findContainingIframe, onIframeResize, scrollToComment and use containerRef/commentIdToScrollTo/commentIdFromHash to ensure the guard only flips when the scroll has executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/comments-ui/src/components/content/hooks/use-comment-navigation.ts`:
- Around line 236-274: The bug is that lastScrolledCommentId.current is set
before the deferred iframe resize callback runs, which lets a rerender cancel
the pending callback and incorrectly suppress future scrolls; to fix, don’t
assign lastScrolledCommentId.current before calling onIframeResize — instead
move the assignment into the onIframeResize callback (after scrollToComment
runs) so lastScrolledCommentId is only updated when the actual iframe-scroll
completes; update the logic around findContainingIframe, onIframeResize,
scrollToComment and use containerRef/commentIdToScrollTo/commentIdFromHash to
ensure the guard only flips when the scroll has executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1a19751-ea28-4f9c-a431-cd7664098003
📒 Files selected for processing (3)
apps/comments-ui/src/components/content/hooks/use-comment-navigation.tsapps/comments-ui/src/components/content/replies.tsxapps/comments-ui/test/unit/components/content/content.test.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/comments-ui/src/components/content/replies.tsx
…threading-styles-wip
no issue The comments UI changes need a patch package release after merging the latest upstream main changes.
kevinansfield
changed the title
This was referenced May 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes https://linear.app/ghost/issue/BER-3649/