Removed comment tombstone serialization #25715
Conversation
WalkthroughDeleted-comment tombstone construction and early-return handling were removed from the comment serializer; deleted comments are now mapped via the standard comment path and, after mapping, have Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
We shouldn't include the content for deleted comments in the responses. I've verified that this already works in my branch.
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js
Show resolved
Hide resolved
rob-ghost
force-pushed
the
remove-comment-tombstones
branch
from
7d768e1 to
64cd2ba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js (1)
93-103: Global redaction of deleted comment HTML looks correctThe additional
status === 'deleted'guard ensures deleted comments never exposehtmlon any endpoint (including admin), while keeping public behavior unchanged. The small overlap with the publicisPublicRequestblock is acceptable and improves safety.ghost/core/core/server/models/comment.js (1)
64-85:browseAllbranching and status filtering align with test expectationsThe new
applyCustomQuerysplit betweenbrowseAlland default behavior cleanly separates:
- Flat moderation view (
browseAll): exclude deleted (and hidden for non-admin) without preserving parents.- Threaded by-post view: keep parents when they have non-excluded replies.
Adding
browseAlltopermittedOptionswires this through correctly. Behavior matches the admin/members e2e tests around deleted/hidden parents and replies.You may want to centralize the
['hidden', 'deleted']/['deleted']status lists (shared withcountRelations().replies) to avoid future drift, but it's not pressing.Also applies to: 319-331
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/comments.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
apps/comments-ui/src/app-context.ts(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js(1 hunks)ghost/core/core/server/models/comment.js(2 hunks)ghost/core/core/server/services/comments/CommentsService.js(1 hunks)ghost/core/test/e2e-api/admin/comments.test.js(4 hunks)ghost/core/test/e2e-api/members-comments/comments.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/comments/CommentsService.js
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-18T10:56:19.906Z
Learnt from: 55sketch
Repo: TryGhost/Ghost PR: 23894
File: ghost/core/core/server/api/endpoints/comments.js:16-22
Timestamp: 2025-06-18T10:56:19.906Z
Learning: The validateCommentData function in ghost/core/core/server/api/endpoints/comments.js is not duplicated elsewhere in the codebase - it's a unique validation function that ensures either post_id or parent_id is provided for comment creation.
Applied to files:
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.jsghost/core/test/e2e-api/admin/comments.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.{ts,tsx} : Prefer less comments and give things clear names
Applied to files:
ghost/core/test/e2e-api/members-comments/comments.test.jsghost/core/test/e2e-api/admin/comments.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/e2e-api/members-comments/comments.test.js
📚 Learning: 2025-05-29T10:37:26.369Z
Learnt from: ErisDS
Repo: TryGhost/Ghost PR: 23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Applied to files:
ghost/core/test/e2e-api/admin/comments.test.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
ghost/core/test/e2e-api/admin/comments.test.js
🧬 Code graph analysis (1)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js (2)
ghost/core/test/e2e-api/admin/comments.test.js (13)
response(753-756)response(783-786)response(816-819)response(850-853)response(904-907)response(936-939)response(967-970)response(997-1000)response(1032-1035)response(1069-1072)response(1118-1121)response(1149-1152)response(1185-1188)ghost/core/test/e2e-api/members-comments/comments.test.js (1)
response(638-641)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Comments-UI tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (7)
apps/comments-ui/src/app-context.ts (1)
15-31: AllowingComment.htmlto be nullable matches API behaviorChanging
htmltostring | nullis consistent with deleted/hidden comments being returned with redacted content. The shape stays stable while signaling removal vianull, andAddComment.htmlcorrectly remains non-nullable.ghost/core/test/e2e-api/members-comments/comments.test.js (2)
657-678: Public deleted-parent behavior and HTML redaction are well coveredThis test now correctly verifies that:
- A deleted parent with published replies remains in the thread,
- Its
statusisdeleted,- Its
htmlisnullwhilememberandrepliesare present.That matches the new “full structure + null html” contract for deleted comments on the public API.
773-793: Reply count expectations with mixed reply statuses look accurateThe updated assertion that only one reply is returned (and matched via
commentMatcherWithReplies({replies: 1})) correctly reflects that hidden/deleted replies are excluded from both the visible replies array and the replies count for public members.ghost/core/test/e2e-api/admin/comments.test.js (4)
442-459: Post-level browse correctly omits fully deleted threadsThis test ensures that a deleted parent with only deleted replies is omitted entirely from the admin post-level view, which matches the query logic (no non-deleted replies to keep the thread around). Looks good.
461-480: Post-level browse retains deleted parents with visible replies while redacting contentThe expectations here—deleted parent included with:
html === null,- a published reply whose
htmlis preserved—accurately capture the “keep thread structure, redact deleted body” behavior for the admin post-level view.
616-633: GET-by-id behavior for deleted comments is clearly specifiedThis test defines the contract that
GET /comments/:id/returns:
- full comment shape (including
memberandreplies),status: 'deleted',html: null.That aligns with the serializer change and ensures admin clients can still render placeholders for deleted comments without data-shape divergence.
1305-1322: Browse All moderation view correctly excludes deleted parentsThe
Browse Alltest confirms that, even when a deleted parent has a published reply, only the reply appears in the flat moderation list (/comments/), while the deleted parent is excluded. This cleanly distinguishes the flat moderation view from the threaded post-level view and matches the newbrowseAllquery branch.
ref #25700 Tombstones were introduced to return minimal data for deleted comments that still had replies, but this approach had issues - the serialized tombstones were missing expected attributes like `member` and `replies`. Instead of returning partial data: - Both Admin and Public APIs preserve thread structure by including deleted parents when they have published replies - Deleted comments return full attributes with `html: null` so the UI can show appropriate placeholder text - The new Browse All endpoint (`/comments`) uses `browseAll: true` to exclude deleted comments entirely (flat list, no thread preservation)
rob-ghost
force-pushed
the
remove-comment-tombstones
branch
from
64cd2ba to
d66ae22
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/test/e2e-api/members-comments/comments.test.js (1)
657-669: Consider explicitly asserting html is null for deleted parents.The test validates that a deleted parent with published replies is included in the response, but it doesn't explicitly check that
htmlisnullfor the deleted comment. Adding an assertion like the one in lines 712-714 would make the test more thorough.Example:
await testGetComments(`/api/comments/post/${postId}/`, [commentMatcherWithReplies({replies: 1})]); + .expect(({body}) => { + should(body.comments[0].html).eql(null); + });ghost/core/core/server/models/comment.js (1)
280-292: Consider renaming for consistency with applyCustomQuery.In line 283, the variable
excludedCommentStatusesserves the same purpose asexcludedStatusesinapplyCustomQuery(line 66). Renaming it toexcludedStatuseswould improve consistency across the codebase.Apply this diff:
countRelations() { return { replies(modelOrCollection, options) { - const excludedCommentStatuses = options.isAdmin ? ['deleted'] : ['hidden', 'deleted']; + const excludedStatuses = options.isAdmin ? ['deleted'] : ['hidden', 'deleted']; modelOrCollection.query('columns', 'comments.*', (qb) => { qb.count('replies.id') .from('comments AS replies') .whereRaw('replies.parent_id = comments.id') - .whereNotIn('replies.status', excludedCommentStatuses) + .whereNotIn('replies.status', excludedStatuses) .as('count__replies'); }); },ghost/core/test/e2e-api/admin/comments.test.js (1)
461-491: Consider explicitly asserting html is null for the deleted parent.While the test validates that a deleted comment with published replies is included in the response, it doesn't explicitly verify that
htmlisnullfor the deleted parent. Since this is a key aspect of the PR's changes (deleted comments return full data withhtml: null), adding this assertion would strengthen the test.Example:
await adminApi.get('/comments/post/' + postId + '/') .expectStatus(200) .matchBodySnapshot({ comments: [{ ...membersCommentMatcher, replies: [replyMatcher] }] - }); + }) + .expect(({body}) => { + assert.equal(body.comments[0].html, null); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/comments.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
apps/comments-ui/src/app-context.ts(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js(1 hunks)ghost/core/core/server/models/comment.js(2 hunks)ghost/core/core/server/services/comments/CommentsService.js(1 hunks)ghost/core/test/e2e-api/admin/comments.test.js(3 hunks)ghost/core/test/e2e-api/members-comments/comments.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js
- ghost/core/core/server/services/comments/CommentsService.js
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
ghost/core/test/e2e-api/admin/comments.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.{ts,tsx} : Prefer less comments and give things clear names
Applied to files:
ghost/core/test/e2e-api/admin/comments.test.jsghost/core/test/e2e-api/members-comments/comments.test.js
📚 Learning: 2025-06-18T10:56:19.906Z
Learnt from: 55sketch
Repo: TryGhost/Ghost PR: 23894
File: ghost/core/core/server/api/endpoints/comments.js:16-22
Timestamp: 2025-06-18T10:56:19.906Z
Learning: The validateCommentData function in ghost/core/core/server/api/endpoints/comments.js is not duplicated elsewhere in the codebase - it's a unique validation function that ensures either post_id or parent_id is provided for comment creation.
Applied to files:
ghost/core/test/e2e-api/admin/comments.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/e2e-api/members-comments/comments.test.js
🧬 Code graph analysis (1)
ghost/core/test/e2e-api/admin/comments.test.js (3)
ghost/core/test/e2e-api/members-comments/comments.test.js (8)
dbFns(18-102)dbFns(884-889)dbFns(899-905)dbFns(913-919)dbFns(1011-1016)dbFns(1036-1041)dbFns(1215-1222)commentMatcher(104-116)ghost/core/core/server/api/endpoints/comments.js (1)
postId(6-6)ghost/core/core/server/services/comments/CommentsController.js (4)
postId(167-167)postId(205-205)postId(248-248)postId(275-275)
🔇 Additional comments (6)
apps/comments-ui/src/app-context.ts (1)
30-30: LGTM! Type correctly reflects null html for deleted comments.The change from
stringtostring | nullaligns with the new behavior where deleted comments have their html explicitly set to null instead of using tombstone representations.ghost/core/test/e2e-api/members-comments/comments.test.js (1)
764-784: LGTM! Test correctly validates deleted parent behavior with mixed reply statuses.The test properly verifies that a deleted parent with published replies is included in the response with only the published reply visible (excluding hidden and deleted replies). The comments and assertions accurately reflect the expected behavior.
ghost/core/core/server/models/comment.js (2)
65-84: LGTM! Clean implementation of browseAll logic.The refactored
applyCustomQuerycorrectly implements two distinct behaviors:
browseAll: trueprovides a flat list excluding deleted comments (for admin moderation)browseAll: false(default) preserves thread structure by including deleted parents that have published repliesThe subquery logic properly checks for non-excluded replies when determining whether to include a deleted parent.
324-331: LGTM! browseAll properly registered as permitted option.The addition of
browseAllto the permitted options list is necessary for the new functionality and follows the existing pattern established byparentIdandisAdmin.ghost/core/test/e2e-api/admin/comments.test.js (2)
442-459: LGTM! Test correctly validates exclusion of fully-deleted threads.The test properly verifies that a deleted parent with only deleted replies is completely excluded from browse results, which aligns with the PR's thread-structure preservation logic (no published replies means no need to include the parent).
1297-1315: LGTM! Test correctly validates Browse All exclusion behavior.The test properly verifies that in Browse All mode (used for admin moderation), deleted comments are excluded entirely even when they have published replies. Only the published reply appears in the flat list, which aligns with the PR's objectives for providing a non-threaded admin view.
| if (options.browseAll) { | ||
| // Browse All: simply exclude statuses, no thread structure preservation | ||
| qb.whereNotIn('comments.status', excludedStatuses); |
There was a problem hiding this comment.
Non blocker: It would be great to find a more long-term solution to this behaviour switch that will scale/be less likely to bite us as we build out more features.
There was a problem hiding this comment.
Yep I have a few ideas but I didn't want to bloat this PR with trying to solve it now.
It will boil down to removing the flags and having something more composable which each individual use case can build upon, as theres:
- What status to include
- If we should be including threaded replies or not
- Should we be including roots or leaves
Depends on which API you're using and we're getting by with flags but they're a major 🚩
Why?
PR #25700 introduced "tombstones" - a pattern where deleted comments with replies would be returned with minimal data (
id,parent_id,statusonly). This was intended to preserve thread structure while hiding deleted content.However, tombstones caused issues because they were missing expected attributes like
memberandreplies, which broke assumptions in consuming code and made the API response inconsistent.What does it do?
Both Admin and Public APIs now preserve thread structure by including deleted parents when they have published replies. Deleted comments return full attributes with
html: null, so the UI can show appropriate placeholder text ("This comment has been removed").New Browse All endpoint (
GET /comments/): UsesbrowseAll: trueflag to exclude deleted comments entirely. This provides a flat list without thread preservation, suitable for admin moderation tables.Changes
html: null(content never exposed)browseAlloption for the new/commentsendpoint to exclude deleted comments