Skip to content

🎨 Improved child display in thread sidebar#26250

Merged
kevinansfield merged 2 commits intomainfrom
issue/BER-3276-improve-child-display-in-thread-sidebar
Feb 5, 2026
Merged

🎨 Improved child display in thread sidebar#26250
kevinansfield merged 2 commits intomainfrom
issue/BER-3276-improve-child-display-in-thread-sidebar

Conversation

@kevinansfield
Copy link
Member

@kevinansfield kevinansfield commented Feb 4, 2026

ref https://linear.app/ghost/issue/BER-3276

Changed the thread sidebar to act as a unified filter view showing the selected comment and its relevant children

  • Renamed comments inReplyTo relation to in_reply_to - removes camelCase/snake_case inconsistency
  • Added useThreadComments hook with NQL filter to fetch only direct replies
  • Thread sidebar can now open for any comment (not just top-level)
  • "Replied to" links use URL navigation to open threads for referenced comments
  • Uses count.direct_replies for accurate reply counts in thread view

Summary by CodeRabbit

  • New Features

    • Thread sidebar: view a selected comment and its direct replies in a dedicated pane.
    • Thread-focused comment loader and public helper to build/share thread links.
  • Improvements

    • Clickable reply metrics and replied-to links open focused thread views while preserving search context.
    • Replied-to context displays appropriately for selected replies; reply counts and related member/post info are more complete.
  • Tests

    • End-to-end tests added to cover thread navigation and sidebar behavior.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Adds a thread-focused comment fetch hook and centralizes thread link construction; refactors frontend to drive thread-view navigation via generated links (removing callback-based navigation), renames backend relation from inReplyToin_reply_to, updates serializers/tests, and adds e2e tests and page helpers for the thread sidebar.

Changes

Cohort / File(s) Summary
Admin API - Thread hook
apps/admin-x-framework/src/api/comments.ts
Added useThreadComments(commentId, options?) calling useBrowseComments with filters for top-level and nested replies, order created_at asc, include member,post,count.direct_replies,count.likes,count.reports,parent,in_reply_to, and limit=100. Also added in_reply_to to useReadComment default include.
Frontend - Metrics & Thread link helper
apps/posts/src/views/comments/components/comment-metrics.tsx
Added exported buildThreadLink(searchParams, commentId); Metric accepts optional to?: string & testId?: string; replies metric now uses to link when available and unified isClickable tooltip; removed onRepliesClick.
Frontend - Thread list & selection flow
apps/posts/src/views/comments/components/comment-thread-list.tsx
Switched to selection-based props (selectedComment, selectedCommentId, isSelectedComment); removed onThreadClick; uses buildThreadLink; nested rows receive selectedCommentId; commentWithReplies built from selectedComment.
Frontend - Thread sidebar (container)
apps/posts/src/views/comments/components/comment-thread-sidebar.tsx
Replaced useCommentReplies with useThreadComments; removed onThreadClick prop; combined loading/error states; renames/data flow change to use selectedComment and threadReplies; passes selection to CommentThreadList.
Frontend - Main comments list
apps/posts/src/views/comments/components/comments-list.tsx
Removed inline handlers and callback-based thread openers; uses buildThreadLink for replied-to links; closing sidebar removes only thread param; removed onRepliesClick/onThreadClick props.
E2E - Helpers & Tests
e2e/helpers/pages/admin/comments/comments-page.ts, e2e/tests/admin/comments/thread-sidebar.test.ts
Added thread sidebar locators and interaction helpers; new Playwright tests covering opening threads from root/reply/replied-to, navigation within threads, URL focus updates, and replied-to visibility.
Backend - Relation naming & includes
ghost/core/core/server/models/comment.js, ghost/core/core/server/api/endpoints/comment-replies.js, ghost/core/core/server/services/comments/comments-service.js
Renamed relation/method from inReplyToin_reply_to; added in_reply_to to allowed includes and updated service/endpoint usages.
Backend - Serializers & Tests
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js, ghost/core/test/unit/api/canary/utils/serializers/output/mapper.test.js
Serializer logic tightened to prefer in_reply_to_id and avoid overwriting valid IDs; tests updated to use in_reply_to/in_reply_to_id.

Sequence Diagram

sequenceDiagram
    participant User
    participant UI as Comments UI
    participant Router as URL/Navigation
    participant Sidebar as CommentThreadSidebar
    participant API as useThreadComments
    participant List as CommentThreadList

    User->>UI: Click replies metric
    UI->>UI: buildThreadLink(searchParams, commentId)
    UI->>Router: Navigate with thread param
    Router->>Sidebar: Mount with thread param
    Sidebar->>API: Fetch threadComments(commentId)
    API-->>Sidebar: Return threadReplies
    Sidebar->>List: render(selectedComment, selectedCommentId, threadReplies)
    List-->>User: Show selected comment + replies
Loading

Possibly related PRs

Poem

🐰
I hop to links and follow threads,
From metric click to sidebar beds.
Replies unfurl in tidy rows,
A rabbit cheers where thread-link goes.
Nibble, hop, and happy code! 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title '🎨 Improved child display in thread sidebar' partially relates to the changeset but is overly vague and misses the main architectural changes (thread view refactoring, relation renaming, URL-based navigation). Consider a more specific title that better captures the primary change, such as 'Refactor thread sidebar to support any comment with URL-based navigation' or 'Add useThreadComments hook and standardize comment relations naming'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch issue/BER-3276-improve-child-display-in-thread-sidebar

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2026

E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 21691597768 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

@kevinansfield kevinansfield force-pushed the issue/BER-3276-improve-child-display-in-thread-sidebar branch from d4307af to bd9a166 Compare February 5, 2026 12:36
@kevinansfield kevinansfield marked this pull request as ready for review February 5, 2026 12:38
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

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/comment-thread-sidebar.tsx (1)

55-75: ⚠️ Potential issue | 🔴 Critical

Fix undefined parentComment reference (runtime error).

parentComment is no longer defined after the selected-comment refactor, so this block will throw. Use selectedComment instead.

🛠️ Proposed fix
-                {parentComment?.post && (
+                {selectedComment?.post && (
                     <>
                         <div className="flex items-center gap-4">
                             <div className="min-w-0 flex-1">
                                 <h3 className="line-clamp-1 text-xl font-semibold text-foreground">
-                                    {parentComment.post.title}
+                                    {selectedComment.post.title}
                                 </h3>
-                                {parentComment.post.excerpt && (
+                                {selectedComment.post.excerpt && (
                                     <p className="mt-1 line-clamp-2 text-sm text-muted-foreground">
-                                        {parentComment.post.excerpt}
+                                        {selectedComment.post.excerpt}
                                     </p>
                                 )}
                             </div>
-                            {parentComment.post.feature_image && (
+                            {selectedComment.post.feature_image && (
                                 <img
-                                    alt={parentComment.post.title || 'Post feature image'}
+                                    alt={selectedComment.post.title || 'Post feature image'}
                                     className="hidden aspect-video h-18 shrink-0 rounded object-cover lg:block"
-                                    src={parentComment.post.feature_image}
+                                    src={selectedComment.post.feature_image}
                                 />
                             )}
                         </div>
                         <Separator className="-mx-6 my-6 w-auto" />
                     </>
                 )}
🤖 Fix all issues with AI agents
In `@apps/posts/src/views/comments/components/comment-thread-list.tsx`:
- Around line 37-40: The UI reads comment.count.direct_replies (used by
hasReplies and metrics) but the thread query only requests count.replies,
leaving direct_replies undefined and inflating nested counts; update the query
used by useThreadComments to request count.direct_replies (in addition to
count.replies) so comment.count.direct_replies is populated, then verify
components referencing hasReplies, comment.count.direct_replies and
comment.count.replies still behave correctly; locate the query definition in
useThreadComments and the API query helper in
apps/admin-x-framework/src/api/comments.ts to add the direct_replies field.

In `@ghost/core/core/server/models/comment.js`:
- Around line 45-47: Update the unit test fixtures that mock Comment model JSON
to use snake_case in_reply_to instead of camelCase inReplyTo: locate the
serializer/mapper tests that supply mocked Comment objects to the mapper (the
tests referencing the mapper) and change any inReplyTo properties to in_reply_to
so they match the Bookshelf toJSON output and the mapper's expected in_reply_to
field (the relation method in_reply_to() is already renamed correctly).
🧹 Nitpick comments (2)
e2e/helpers/pages/admin/comments/comments-page.ts (2)

19-42: Potential selector conflict between threadSidebar and disableCommentsModal.

Both threadSidebar (line 40) and disableCommentsModal (line 34) use page.getByRole('dialog'). If both dialogs could theoretically be present or if tests interact with both, this could cause flaky behavior.

Consider differentiating using a more specific selector or data-testid:

-        this.threadSidebar = page.getByRole('dialog');
+        this.threadSidebar = page.getByRole('dialog', {name: 'Thread'});

Alternatively, if the Ghost codebase doesn't have distinguishing attributes on these dialogs, consider suggesting a data-testid="thread-sidebar" be added to the component.


90-96: Parent traversal in locators may be brittle.

The getRepliedToLink and getMainListRepliedToLink methods use .locator('..') for parent traversal, which couples tests to DOM structure. This could break if the markup changes.

If this becomes problematic, consider requesting a data-testid on the replied-to link container in the Ghost codebase for more stable selection.

Copy link
Member

@jonatansberg jonatansberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Added a few comments that we could either adress now or treat as followups if we want to get this merged pronto.

searchParams: {
filter: `(parent_id:${commentId}+in_reply_to_id:null),in_reply_to_id:${commentId}`,
order: 'created_at asc',
include: 'member,post,count.replies,count.likes,count.reports,parent,in_reply_to',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this say count.direct_replies?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would explain an inconsistency I saw too!

icon: React.ReactNode;
count: number;
label: string;
link?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd probably name this to to align with the React Router Link conventions.

Comment on lines 37 to 39
// Use direct_replies for checking if a reply has further replies (reply-to-replies)
// Use replies or total_replies for root comments, direct_replies for reply comments
const hasReplies = (comment.replies?.length ?? 0) > 0 || (comment.count?.direct_replies ?? comment.count?.replies ?? 0) > 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is outdated (there aren't any total_replies)

/>

{comment.in_reply_to_snippet && !isReply && (
{comment.in_reply_to_snippet && (isSelectedComment || (comment.in_reply_to_id && comment.in_reply_to_id !== selectedCommentId)) && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name this (combination of) criteria something so that the intent here is clearer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified further I think because we're only showing direct replies in the sidebar now. It's only the selected comment that needs the replied-to line

await expect(commentsPage.getThreadCommentByText('Reply to first level reply')).toBeVisible();
});

test('url structure uses only thread param without reply_to', async ({page}) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test name makes no sense other than in the context of an agent conversation. I think we can scrap the full test case?

await expect(selectedComment.getByText('Replied to:')).toBeVisible();
});

test('shows replied to line for non-direct children in thread', async ({page}) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read that as "shows that line on the left" not that it shows the "reply to context"

await expect(nonDirectReply.getByText('Replied to:')).toBeVisible();
});

test('hides replied to line for direct children in thread', async ({page}) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, maybe we can merge this with the test above?

const replyToComment = jsonModel.inReplyTo || (!isPublicRequest && jsonModel.parent_id && jsonModel.parent);
// Use in_reply_to if available, otherwise fall back to parent for first-level replies only
// (first-level replies have parent_id but no in_reply_to_id)
const replyToComment = jsonModel.in_reply_to || (!isPublicRequest && jsonModel.parent_id && !jsonModel.in_reply_to_id && jsonModel.parent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this extra condition? Seems like we would already hit the in_reply_to on the left hand side

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude thinks yes but there could still be something to tidy up here 🤔


The in_reply_to relation is based on in_reply_to_id (see comment.js:45-46):

in_reply_to() {
    return this.belongsTo('Comment', 'in_reply_to_id');
},

For first-level replies (direct replies to a top-level comment), in_reply_to_id is NULL. They only have a parent_id. So in_reply_to will also be null for these.

The fallback handles this case for admin requests:

  • !isPublicRequest - admin only
  • jsonModel.parent_id - it's a reply
  • !jsonModel.in_reply_to_id - it's a first-level reply (not a nested reply)
  • jsonModel.parent - use the parent comment to generate the snippet

Without this fallback, first-level replies in admin would have no in_reply_to_snippet, even though they're replies to something. The admin UI wants to show "replying to..." snippets for ALL replies, not just nested ones.

The && !jsonModel.in_reply_to_id condition isn't needed though? We've already checked that on the left side of the OR

Good question, but the check is still needed. Here's the distinction:

in_reply_to being falsy doesn't mean in_reply_to_id is null. Consider:

  1. in_reply_to_id is null → in_reply_to is null (first-level reply)
  2. in_reply_to_id exists but referenced comment was deleted → in_reply_to could be null
  3. in_reply_to_id exists but relation wasn't loaded → in_reply_to is undefined/null

The !jsonModel.in_reply_to_id check protects against cases 2 and 3. If someone replied to a nested comment that was later deleted, we shouldn't fall back to showing the parent (root) comment's snippet - that would be misleading since they didn't reply to the root.

The check ensures we only use the parent fallback for true first-level replies where in_reply_to_id was never set in the first place.

ref https://linear.app/tryghost/issue/BER-3276

Follows the convention used by other model relations (snake_case)
@kevinansfield kevinansfield force-pushed the issue/BER-3276-improve-child-display-in-thread-sidebar branch 2 times, most recently from bdf119b to f40c0fb Compare February 5, 2026 13:47
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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/comment-thread-sidebar.tsx (1)

55-78: ⚠️ Potential issue | 🔴 Critical

Critical bug: parentComment is undefined.

The code references parentComment (lines 55, 60, 62, 63, 64, 68, 70, 72) but this variable is never defined. Based on the refactoring that renamed parentComment to selectedComment (line 44), all these references need to be updated.

This will cause a ReferenceError at runtime, crashing the component.

🐛 Proposed fix
-                {parentComment?.post && (
+                {selectedComment?.post && (
                     <>
                         <div className="flex items-center gap-4">
                             <div className="min-w-0 flex-1">
                                 <h3 className="line-clamp-1 text-xl font-semibold text-foreground">
-                                    {parentComment.post.title}
+                                    {selectedComment.post.title}
                                 </h3>
-                                {parentComment.post.excerpt && (
+                                {selectedComment.post.excerpt && (
                                     <p className="mt-1 line-clamp-2 text-sm text-muted-foreground">
-                                        {parentComment.post.excerpt}
+                                        {selectedComment.post.excerpt}
                                     </p>
                                 )}
                             </div>
-                            {parentComment.post.feature_image && (
+                            {selectedComment.post.feature_image && (
                                 <img
-                                    alt={parentComment.post.title || 'Post feature image'}
+                                    alt={selectedComment.post.title || 'Post feature image'}
                                     className="hidden aspect-video h-18 shrink-0 rounded object-cover lg:block"
-                                    src={parentComment.post.feature_image}
+                                    src={selectedComment.post.feature_image}
                                 />
                             )}
                         </div>
                         <Separator className="-mx-6 my-6 w-auto" />
                     </>
                 )}
🧹 Nitpick comments (2)
e2e/helpers/pages/admin/comments/comments-page.ts (2)

34-34: Potential selector collision between threadSidebar and disableCommentsModal.

Both disableCommentsModal (line 34) and threadSidebar (line 40) use page.getByRole('dialog'), which will match any dialog on the page. If both dialogs could be open simultaneously or if tests check visibility, this could cause flaky behavior.

Consider using more specific selectors, such as adding a data-testid to each dialog in the Ghost codebase or scoping by dialog title:

-        this.threadSidebar = page.getByRole('dialog');
+        this.threadSidebar = page.getByRole('dialog', {name: 'Thread'});

Also applies to: 40-40


90-92: DOM traversal with .locator('..') is fragile.

Using .locator('..') to traverse to the parent element is brittle and can break with minor markup changes. Consider adding a data-testid to the "Replied to" link container in the Ghost codebase for more resilient test selectors.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@e2e/helpers/pages/admin/comments/comments-page.ts`:
- Around line 19-21: The thread sidebar locator is colliding with the disable
comments dialog because both use page.getByRole('dialog') unscoped; update the
threadSidebar locator (and any related selectors used by waitForThreadSidebar)
to scope the dialog to a unique ancestor or aria/name/test-id (e.g., use
page.locator('[data-testid="thread-sidebar"]').getByRole('dialog') or
page.getByRole('dialog', { name: 'Thread sidebar' }) so it only matches the
sidebar), and similarly verify and scope disableCommentsModal and cancelButton
selectors so they target the intended dialog to avoid cross-matching.
- Around line 90-92: The getRepliedToLink function uses both XPath parent
traversal (locator('..')) and a CSS selector ('a, button'); replace these with
semantic Playwright locators: in getRepliedToLink(row: Locator) use
row.getByRole('link', { name: /Replied to:/ }) or row.getByRole('button', {
name: /Replied to:/ }) to target the actionable element by its accessible name
(or if the accessible name isn't available, add a data-testid on the
parent/action in the app and use row.getByTestId('replied-to-link')); update the
implementation in the getRepliedToLink function (and any callers) to rely solely
on getByRole or getByTestId rather than locator('..') or locator('a, button').

Comment on lines +90 to +97
getRepliedToLink(row: Locator): Locator {
return row.getByText('Replied to:').locator('..').locator('a, button');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find and examine the actual file
fd "comments-page.ts" --path "*/e2e/helpers/pages/*"

Repository: TryGhost/Ghost

Length of output: 273


🏁 Script executed:

# Read the file around lines 90-92
head -100 e2e/helpers/pages/admin/comments/comments-page.ts | tail -20

Repository: TryGhost/Ghost

Length of output: 689


🏁 Script executed:

# Check if there are more usages to understand context better
head -110 e2e/helpers/pages/admin/comments/comments-page.ts | tail -30

Repository: TryGhost/Ghost

Length of output: 963


🏁 Script executed:

# Search for how this method is used in tests to understand the intended behavior
rg "getRepliedToLink" e2e/

Repository: TryGhost/Ghost

Length of output: 281


🏁 Script executed:

# Check the actual HTML structure or tests to understand what element needs to be selected
cat -n e2e/tests/admin/comments/thread-sidebar.test.ts | grep -A 5 -B 5 "getRepliedToLink"

Repository: TryGhost/Ghost

Length of output: 703


🏁 Script executed:

# Check if there's a data-testid or accessible role we could use instead
# Search for the HTML structure or context around "Replied to:" text
rg "Replied to:" e2e/ -A 3 -B 3

Repository: TryGhost/Ghost

Length of output: 2911


Remove both CSS and XPath selectors; use only semantic locators.

The code uses two selector types that violate the semantic locator guideline:

  1. locator('..') — an XPath selector for parent traversal
  2. locator('a, button') — a CSS selector

The suggested fix addresses the CSS selector but leaves the XPath selector. Use semantic locators throughout:

♻️ Improved semantic locator
    getRepliedToLink(row: Locator): Locator {
-       return row.getByText('Replied to:').locator('..').locator('a, button');
+       const repliedTo = row.getByText('Replied to:').locator('..');
+       return repliedTo.getByRole('link').or(repliedTo.getByRole('button'));
    }

If locator('..') cannot be replaced with a semantic alternative, consider adding data-testid to the parent element in the Ghost codebase.

As per coding guidelines, never use CSS or XPath selectors in page objects—only semantic locators or data-testid.

🤖 Prompt for AI Agents
In `@e2e/helpers/pages/admin/comments/comments-page.ts` around lines 90 - 92, The
getRepliedToLink function uses both XPath parent traversal (locator('..')) and a
CSS selector ('a, button'); replace these with semantic Playwright locators: in
getRepliedToLink(row: Locator) use row.getByRole('link', { name: /Replied to:/
}) or row.getByRole('button', { name: /Replied to:/ }) to target the actionable
element by its accessible name (or if the accessible name isn't available, add a
data-testid on the parent/action in the app and use
row.getByTestId('replied-to-link')); update the implementation in the
getRepliedToLink function (and any callers) to rely solely on getByRole or
getByTestId rather than locator('..') or locator('a, button').

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2026

E2E Tests Failed

To view the Playwright test report locally, run:

REPORT_DIR=$(mktemp -d) && gh run download 21713927816 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR"

@kevinansfield kevinansfield force-pushed the issue/BER-3276-improve-child-display-in-thread-sidebar branch from f40c0fb to 21354f6 Compare February 5, 2026 14:39
ref https://linear.app/tryghost/issue/BER-3276

- Added useThreadComments hook with NQL filter to fetch only direct replies
- Thread sidebar can now open for any comment (not just top-level)
- "Replied to" links use URL navigation to open threads for referenced comments
- Uses count.direct_replies for accurate reply counts in thread view
@kevinansfield kevinansfield force-pushed the issue/BER-3276-improve-child-display-in-thread-sidebar branch from 21354f6 to abb9c30 Compare February 5, 2026 14:43
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@e2e/helpers/pages/admin/comments/comments-page.ts`:
- Around line 91-93: The getThreadCommentById method currently uses a CSS
attribute selector which violates the E2E guideline to use semantic/test-id
locators; update the UI to add a combined data-testid for each thread row (e.g.,
data-testid="comment-thread-row-<id>") and change getThreadCommentById to use
the Playwright semantic locator (page.getByTestId with
`comment-thread-row-${commentId}`) instead of the CSS attribute selector so
tests rely only on data-testid locators.
🧹 Nitpick comments (2)
e2e/tests/admin/comments/thread-sidebar.test.ts (1)

30-228: Make Arrange/Act/Assert sections explicit for readability.

Consider minimal section markers or helper blocks to clearly separate setup, action, and assertion in each test while keeping comments light.
As per coding guidelines, use the AAA Pattern (Arrange, Act, Assert) in E2E tests with clear sections.

apps/posts/src/views/comments/components/comment-thread-list.tsx (1)

68-81: Consider guarding against an empty link target.

If neither comment.in_reply_to_id nor comment.parent_id is set (data inconsistency), buildThreadLink returns undefined and the fallback || '' results in to="", which could cause unexpected navigation behavior.

Consider conditionally rendering the Link only when a valid target exists:

♻️ Suggested guard
-                        {comment.in_reply_to_snippet && isSelectedComment && (
+                        {comment.in_reply_to_snippet && isSelectedComment && (comment.in_reply_to_id || comment.parent_id) && (
                             <div className={`mb-1 line-clamp-1 text-sm ${comment.status === 'hidden' && 'opacity-50'}`}>
                                 <span className="text-muted-foreground">Replied to:</span>&nbsp;
                                 <Link
                                     className="text-sm font-normal text-muted-foreground hover:text-foreground"
-                                    to={buildThreadLink(searchParams, comment.in_reply_to_id || comment.parent_id) || ''}
+                                    to={buildThreadLink(searchParams, comment.in_reply_to_id || comment.parent_id)!}
                                     onClick={(e: React.MouseEvent) => {
                                         e.stopPropagation();
                                     }}

Comment on lines +91 to +93
getThreadCommentById(commentId: string): Locator {
return this.page.locator(`[data-testid="comment-thread-row"][data-comment-id="${commentId}"]`);
}
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the full context of the comments-page.ts file
wc -l e2e/helpers/pages/admin/comments/comments-page.ts

Repository: TryGhost/Ghost

Length of output: 111


🏁 Script executed:

# Read the relevant section with more context
sed -n '80,110p' e2e/helpers/pages/admin/comments/comments-page.ts

Repository: TryGhost/Ghost

Length of output: 805


🏁 Script executed:

# Check class declaration and other locator methods for consistency
head -50 e2e/helpers/pages/admin/comments/comments-page.ts

Repository: TryGhost/Ghost

Length of output: 2611


🏁 Script executed:

# Search for usage of getThreadCommentById in tests
rg "getThreadCommentById" e2e

Repository: TryGhost/Ghost

Length of output: 571


🏁 Script executed:

# Look for the comment component or related UI files
find . -path ./node_modules -prune -o -type f \( -name "*comment*" -o -name "*thread*" \) \( -name "*.ts" -o -name "*.tsx" -o -name "*.hbs" \) -print | head -20

Repository: TryGhost/Ghost

Length of output: 1307


🏁 Script executed:

# Check the comment thread sidebar component
cat -n apps/posts/src/views/comments/components/comment-thread-sidebar.tsx | head -150

Repository: TryGhost/Ghost

Length of output: 5679


🏁 Script executed:

# Search for where comment-thread-row data-testid is used/rendered
rg "comment-thread-row" --type tsx --type ts -B 2 -A 2

Repository: TryGhost/Ghost

Length of output: 85


🏁 Script executed:

# Check the test file to understand the usage pattern better
sed -n '1,50p' e2e/tests/admin/comments/thread-sidebar.test.ts

Repository: TryGhost/Ghost

Length of output: 1948


🏁 Script executed:

# Find and check the comment-thread-list component
find . -name "comment-thread-list.*" -type f | head -5

Repository: TryGhost/Ghost

Length of output: 124


🏁 Script executed:

# Check how comment rows are rendered in the thread list
cat -n ./apps/posts/src/views/comments/components/comment-thread-list.tsx | head -100

Repository: TryGhost/Ghost

Length of output: 5651


Avoid CSS selectors in page objects; prefer semantic/test-id locators.

The current implementation uses a chained CSS attribute selector to target both data-testid and data-comment-id. Consider adding a combined data-testid (e.g., data-testid="comment-thread-row-{commentId}") in the UI and updating this method to use getByTestId() instead. As per coding guidelines, never use CSS or XPath selectors—only semantic locators or data-testid in E2E tests.

🔧 Suggested update (requires adding a matching data-testid in the UI)
-    getThreadCommentById(commentId: string): Locator {
-        return this.page.locator(`[data-testid="comment-thread-row"][data-comment-id="${commentId}"]`);
-    }
+    getThreadCommentById(commentId: string): Locator {
+        return this.page.getByTestId(`comment-thread-row-${commentId}`);
+    }
🤖 Prompt for AI Agents
In `@e2e/helpers/pages/admin/comments/comments-page.ts` around lines 91 - 93, The
getThreadCommentById method currently uses a CSS attribute selector which
violates the E2E guideline to use semantic/test-id locators; update the UI to
add a combined data-testid for each thread row (e.g.,
data-testid="comment-thread-row-<id>") and change getThreadCommentById to use
the Playwright semantic locator (page.getByTestId with
`comment-thread-row-${commentId}`) instead of the CSS attribute selector so
tests rely only on data-testid locators.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a fix for this but I'll create a separate PR once this is merged so we don't have to wait for CI all over again just for a test improvement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinansfield, sounds good! Thank you for acknowledging this and planning to address it in a follow-up PR. That's a practical approach to avoid the CI overhead. Looking forward to seeing the fix! 👍


🧠 Learnings used
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 26219
File: ghost/core/test/e2e-api/admin/comments.test.js:630-676
Timestamp: 2026-02-04T15:58:05.895Z
Learning: In the Ghost comments API (ghost/core/test/e2e-api/admin/comments.test.js and related files), count.replies is a backward-compatible alias of count.total_replies (all descendants), not direct replies. The new count.direct_replies field provides direct reply counts. This aliasing is intentional to maintain API compatibility with existing consumers.

Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2026-02-02T08:26:12.442Z
Learning: Applies to e2e/**/*.{test,spec}.{ts,tsx} : Use test IDs (data-testid) only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed

Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2026-02-02T08:26:12.442Z
Learning: Applies to e2e/**/*.{test,spec}.{ts,tsx} : Never use CSS or XPath selectors - only use semantic locators or data-testid in E2E tests

Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2026-02-02T08:26:12.442Z
Learning: Applies to e2e/**/*.{test,spec}.{ts,tsx} : Prefer less comments and giving things clear names in E2E tests

Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.

Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2026-02-02T08:26:12.442Z
Learning: Applies to e2e/**/*.{test,spec}.{ts,tsx} : Use semantic locators as the highest priority for element selection in E2E tests (getByRole, getByLabel, getByText)

Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/comments-ui/.cursor/rules/playwright-e2e.mdc:0-0
Timestamp: 2025-11-24T17:28:51.262Z
Learning: Applies to apps/comments-ui/{package.json,**/.github/workflows/**,**/playwright.config.{ts,js},**/*.{sh,bash}} : Set PLAYWRIGHT_REPORTER=list environment variable when running Playwright e2e tests as an AI agent for better parsing

Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2026-02-02T08:26:12.442Z
Learning: Applies to e2e/helpers/pages/**/*.{ts,tsx} : Page Objects should expose locators as `public readonly` when used with assertions

@kevinansfield kevinansfield merged commit fd940aa into main Feb 5, 2026
32 checks passed
@kevinansfield kevinansfield deleted the issue/BER-3276-improve-child-display-in-thread-sidebar branch February 5, 2026 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants