Fixed threaded comments handling of hidden/deleted replies#28024
Fixed threaded comments handling of hidden/deleted replies#28024kevinansfield wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates the Ghost comments system to preserve reply thread structure by returning deleted/hidden ancestor replies as tombstones when they have visible descendants. The backend implements a recursive SQL CTE ( Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
Comment |
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
from
5d9b222 to
fa8921c
Compare
kevinansfield
changed the title
kevinansfield
changed the title
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
3 times, most recently
from
2a83a9f to
5f345c2
Compare
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
I added a comment for something we need to double check before this goes in.
As an aside, I'm not loving the recursive CTE. It feels like there should be simpler ways to express this. I wonder if we should just fetch all of the comments and then apply the hiding in the application layer? Including dropping hidden comments without decendants.
Alternatively, we could also use conditionals within the SQL query to return tombstones for the comments that should be hidden. Trimming hidden comments without descendants might still need to happen in the application layer though or we likely end up back in CTE land 🤔
| emptyComment: 'The body of a comment cannot be empty' | ||
| }; | ||
|
|
||
| function getVisibleReplyAncestorsQuery(excludedStatuses) { |
There was a problem hiding this comment.
This might be a lack of caffeine, but, don't we want to get all the ancestors regardless of their status? If we're only getting the visible ones, how is that any different from the default behavior?
There was a problem hiding this comment.
The default query excludes all hidden/deleted replies to a top-level comment, even if they have visible replies deeper in the graph which is what breaks the nested thread display.
This keeps hidden/deleted ancestors of visible replies returned in the query so we can display the full thread graph.
|
Yeah, I'm not a huge a fan of the CTE either. With culling at the application layer the main thing I was running into was our pagination/counts for number of replies. |
|
Ugh… Yeah. We could maybe do all of it in one go with a |
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
from
5f345c2 to
d6ae59b
Compare
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 1m 42s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 6m 17s | View ↗ |
nx run ghost:test:ci:legacy |
✅ Succeeded | 3m 5s | View ↗ |
nx build @tryghost/portal |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/signup-form |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/activitypub |
✅ Succeeded | 2s | View ↗ |
nx build @tryghost/comments-ui |
✅ Succeeded | 1s | View ↗ |
nx build @tryghost/sodo-search |
✅ Succeeded | <1s | View ↗ |
Additional runs (8) |
✅ Succeeded | ... | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-05-26 14:33:26 UTC
![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
🤖 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 `@ghost/core/core/server/models/comment.js`:
- Around line 10-31: The recursive CTE in getVisibleReplyAncestorsQuery walks
the entire comments table; change it to seed the recursion from the current
reply id set and only recurse from those ancestors. Update
getVisibleReplyAncestorsQuery to accept the current reply IDs (or add a second
parameter like replyIds) and use those IDs as the initial SELECT (e.g., SELECT
in_reply_to_id FROM comments WHERE id IN (...) AND in_reply_to_id IS NOT NULL
AND status NOT IN (placeholders)), then recurse only from that seed (keep the
excludedStatuses placeholders), and ensure the final SELECT returns only
ancestors derived from that seeded set; apply the same scoping change to the
related CTE usage at the other occurrence noted (lines ~205-211).
🪄 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: bf163855-3adf-4ddf-b45f-083764829de6
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
apps/comments-ui/test/unit/utils/thread-graph.test.tsghost/core/core/server/models/comment.jsghost/core/test/e2e-api/admin/comments.test.jsghost/core/test/e2e-api/members-comments/comments.test.js
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
from
d6ae59b to
b286294
Compare
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 `@ghost/core/core/server/models/comment.js`:
- Around line 261-263: The call to applyRepliesWithRelatedOption for
non-findPage paths omits parentIds, which causes getVisibleReplyAncestorsQuery
to receive an empty parentIds set and skip the ancestor-preservation branch; fix
by passing the relevant parentIds into applyRepliesWithRelatedOption (so
getVisibleReplyAncestorsQuery can evaluate ancestor preservation) — update the
call site that invokes applyRepliesWithRelatedOption(methodName !== 'findPage')
to supply the parentIds (or derive them from options) and adjust
applyRepliesWithRelatedOption signature if needed so
getVisibleReplyAncestorsQuery sees non-empty parentIds and preserves
hidden/deleted ancestors.
🪄 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: 263b94a4-06b5-4713-96fa-b335cdc80c8b
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
apps/comments-ui/test/unit/utils/thread-graph.test.tsghost/core/core/server/models/comment.jsghost/core/test/e2e-api/admin/comments.test.jsghost/core/test/e2e-api/members-comments/comments.test.js
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
from
97ec2af to
d7a938e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ghost/core/core/server/models/comment.js (1)
275-277:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPass parent scope into non-
findPagereply filtering.
applyRepliesWithRelatedOptionis called withoutparentIds, sogetVisibleReplyAncestorsQueryfalls back to an empty set (Line 11-Line 13). That disables ancestor tombstone preservation on non-findPagepaths that eager-loadreplies, creating inconsistent behavior versusfindPage.🤖 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 `@ghost/core/core/server/models/comment.js` around lines 275 - 277, The call to applyRepliesWithRelatedOption inside the non-'findPage' branch omits parentIds, causing getVisibleReplyAncestorsQuery to receive an empty set and drop ancestor tombstone preservation; update the invocation in that branch to pass the parentIds value (i.e. call applyRepliesWithRelatedOption with the same parentIds argument used elsewhere) so getVisibleReplyAncestorsQuery receives the correct parentIds and preserves ancestor tombstones when eager-loading replies.
🤖 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.
Duplicate comments:
In `@ghost/core/core/server/models/comment.js`:
- Around line 275-277: The call to applyRepliesWithRelatedOption inside the
non-'findPage' branch omits parentIds, causing getVisibleReplyAncestorsQuery to
receive an empty set and drop ancestor tombstone preservation; update the
invocation in that branch to pass the parentIds value (i.e. call
applyRepliesWithRelatedOption with the same parentIds argument used elsewhere)
so getVisibleReplyAncestorsQuery receives the correct parentIds and preserves
ancestor tombstones when eager-loading replies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b0f30e5-f38f-4426-8da7-5a327f95f100
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
apps/comments-ui/test/unit/utils/thread-graph.test.tsghost/core/core/server/models/comment.jsghost/core/test/e2e-api/admin/comments.test.jsghost/core/test/e2e-api/members-comments/comments.test.js
✅ Files skipped from review due to trivial changes (1)
- apps/comments-ui/test/unit/utils/thread-graph.test.ts
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
from
d7a938e to
e8b0c91
Compare
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
3 times, most recently
from
a22e205 to
32aceca
Compare
ref https://linear.app/ghost/issue/BER-3677/hidden-comments-collapse-thread-nesting-incorrectly Comment APIs need to return hidden/deleted tombstones when they preserve the path to visible descendants. This uses a post-scoped path-based recursive SQL query for displayable comment IDs, keeps reply counts tied to visible comments, preserves local deleted reply tombstones while descendants are loaded, and bumps Comments UI for release.
kevinansfield
force-pushed
the
codex/comments-thread-structure-sql
branch
2 times, most recently
from
32aceca to
28f3963
Compare
|
Merged as part of c0aa1ce |
2 participants
What changed
post_idinside the CTE so tombstone path resolution stays constrained to the post being browsed.count.repliesandcount.direct_repliesas visible reply counts rather than structural tombstone counts.@tryghost/comments-uito1.5.3.Why
Threaded comments need real server-provided tombstones for hidden/deleted ancestors so visible replies render at the correct nesting level. Doing this in SQL keeps the API response authoritative, avoids client-side tombstone inference, and prevents returning hidden/deleted leaf replies that do not preserve any visible thread structure.