Added Admin API endpoint for browsing all comments #25700
Conversation
WalkthroughAdds an admin-facing "browseAll" comments endpoint and controller handler, a CommentsService.getAdminAllComments method, and an admin GET /comments route (placed before /comments/:id). Serializers for comments now include parent_id and return a minimal tombstone object for comments with status "deleted" (optionally including post relation and recursively mapping replies). E2E tests and matchers were updated to assert parent_id, tombstone shapes, pagination, filtering, ordering, and related behaviors. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)ghost/core/test/e2e-api/members-comments/comments.test.jsThanks 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ghost/core/core/server/services/comments/CommentsService.js (1)
212-234: Consider explicitly settingisAdmin: truefor consistency.Unlike
getAdminAllCommentswhich explicitly setsisAdmin: true, this method relies on the caller to pass it via options. For clarity and defensive coding, consider making this explicit.async getAdminPostComments(options) { const findPageOptions = { ...options, - parentId: null // Only top-level comments for per-post browse + parentId: null, // Only top-level comments for per-post browse + isAdmin: true // Admin context allows viewing hidden comments }; return await this.models.Comment.findPage(findPageOptions); }ghost/core/test/e2e-api/admin/comments.test.js (1)
1151-1388: Browse All admin tests give solid coverage; a couple of edge cases are optional follow‑upsThe new “Browse All” suite does a good job exercising the critical behaviours: cross‑post browsing,
include_nestedsemantics, status/deleted filtering, member filters, pagination, ordering, and relations (member,post). The setup uses the existing helpers consistently, so it stays in line with the rest of the file.If you want to tighten things further (optional), you could:
- Add a case combining
include_nested=falsewith a filter (e.g.status:hiddenormember_id:) to ensure filters always apply to the top‑level subset.- Add a small assertion that the
memberrelation is still present wheninclude=postis used, to guard against future regressions inwithRelatedhandling.These are nice-to-haves; the current tests are already strong.
ghost/core/core/server/services/comments/CommentsController.js (2)
66-99: adminBrowsePost correctly scopes by post_id and preserves admin impersonation behaviourThe method cleanly builds a post‑scoped filter (or
mongoTransformer), deletespost_idfrom options once encoded in the filter (while safely capturing it in a localpostId), setsisAdmin, and reuses#setImpersonationContextbefore delegating togetAdminPostComments. This keeps behaviour aligned with the existing admin browse‑by‑post flow.Optionally, you could add a defensive guard (or assertion) for a missing
frame.options.post_idto fail fast if the route wiring ever changes, but it’s not strictly required given current routing.
101-123: adminBrowseAll cleanly parses frame options; consider mirroring admin flags if needed laterThe new
adminBrowseAllmethod nicely isolates controller concerns: it normalizesinclude_nested(handling both'false'andfalse), provides a defaultorder = 'created_at desc', and passes a trimmed parameter object intogetAdminAllComments, avoiding leaking the wholeframe.If, in future, the browse‑all endpoint needs the same semantics as other admin routes (e.g. exposing
likedfor animpersonate_member_uuidor relying on anisAdminhint in the service), you might want to:
- Set
frame.options.isAdmin = true(or pass an explicitisAdmin: trueflag in the options object), and/or- Call
await this.#setImpersonationContext(frame.options)and thread the resulting context through.For the current moderation use case, the implementation looks sound as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
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!**/*.snapghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
ghost/core/core/server/api/endpoints/comments.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js(1 hunks)ghost/core/core/server/services/comments/CommentsController.js(1 hunks)ghost/core/core/server/services/comments/CommentsService.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/comments.test.js(2 hunks)ghost/core/test/e2e-api/members-comments/comments.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: API routes should be defined in `ghost/core/core/server/api/`
Applied to files:
ghost/core/core/server/web/api/endpoints/admin/routes.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/core/server/web/api/endpoints/admin/routes.jsghost/core/test/e2e-api/admin/comments.test.jsghost/core/test/e2e-api/members-comments/comments.test.jsghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.jsghost/core/core/server/api/endpoints/comments.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.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/admin/comments.test.js
🧬 Code graph analysis (3)
ghost/core/core/server/web/api/endpoints/admin/routes.js (3)
ghost/core/core/server/web/api/endpoints/content/routes.js (3)
router(12-12)mw(5-5)api(3-3)ghost/core/test/legacy/mock-express-style/utils/mock-express.js (1)
http(1-1)ghost/core/core/server/api/endpoints/session.js (1)
api(5-5)
ghost/core/core/server/services/comments/CommentsController.js (2)
ghost/core/core/server/api/endpoints/comments.js (1)
postId(6-6)ghost/core/test/e2e-api/admin/comments.test.js (1)
postId(27-27)
ghost/core/core/server/services/comments/CommentsService.js (1)
ghost/core/core/server/services/comments/CommentsController.js (1)
includeNested(113-113)
⏰ 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). (8)
- GitHub Check: Ghost-CLI tests
- 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: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (7)
ghost/core/core/server/services/comments/CommentsService.js (1)
179-210: Well-structured admin browsing method with clear documentation.The
getAdminAllCommentsmethod correctly:
- Skips the
checkEnabled()call to allow admins to moderate existing comments- Uses
parentId: undefinedvsnullto control nested inclusion- Sets
isAdmin: trueto enable viewing hidden commentsghost/core/core/server/web/api/endpoints/admin/routes.js (1)
44-45: Correct route ordering and authentication.The comment clearly explains why
GET /commentsmust be defined beforeGET /comments/:id. This prevents the parametrized route from incorrectly capturing requests to the browse-all endpoint. Themw.authAdminApimiddleware is correctly applied.ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js (1)
6-16: Fields correctly added to support flat list admin browsing.Adding
post_idandparent_idto the serialized output enables the flat list response structure mentioned in the PR objectives, supporting efficient pagination and bulk operations in the moderation interface.ghost/core/test/e2e-api/members-comments/comments.test.js (1)
104-117: Test matcher correctly updated for new fields.The
commentMatcherappropriately defines:
post_id: anyObjectId— always present on commentsparent_id: nullable(anyObjectId)— null for top-level, ObjectId for repliesThis correctly reflects the serializer changes and maintains test coverage for the new admin browsing functionality.
ghost/core/core/server/api/endpoints/comments.js (2)
96-100: Clean separation of post-specific and site-wide browsing.The rename from
adminBrowsetoadminBrowsePostclearly communicates intent and maintains the existing per-post functionality while the newbrowseAllhandles site-wide browsing.
101-123: Well-designed browseAll endpoint configuration.The endpoint correctly:
- Exposes
include_nestedoption for controlling reply inclusion in the flat list- Reuses
browsepermission method to leverage existing admin authorization- Uses empty validation since no required parameters (unlike post-specific browse which requires
post_id)ghost/core/test/e2e-api/admin/comments.test.js (1)
9-16: Extending membersCommentMatcher to include post_id and nullable parent_id is consistent with the new API shapeAsserting
post_id: anyObjectIdandparent_id: nullable(anyObjectId)on member-facing comment payloads matches the new serializer contract and keeps snapshots honest about the new fields. No issues here.
rob-ghost
force-pushed
the
rob/comments-browse-all-api
branch
10 times, most recently
from
12b7a27 to
9e5a55c
Compare
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20140535270 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 similar comment
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20140535270 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
rob-ghost
force-pushed
the
rob/comments-browse-all-api
branch
from
9e5a55c to
30d820f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/services/comments/CommentsService.js (1)
212-217: Consider consistency withgetAdminAllCommentsregardingcheckEnabled().Note that
getAdminCommentsstill callscheckEnabled()(Line 213) while the newgetAdminAllCommentsintentionally skips it. If the reasoning "admins can moderate existing comments" applies, you may want to aligngetAdminCommentsin a follow-up. This is not blocking for this PR.ghost/core/test/e2e-api/admin/comments.test.js (1)
1219-1431: Comprehensive test coverage for the new Browse All endpoint.The test suite thoroughly covers:
- Cross-post browsing
- Flat list with/without nested replies
- Hidden and deleted comment handling
- Tombstone semantics
- Status and member filtering
- Pagination and ordering
One enhancement to consider: the PR objectives mention filtering by
postbut there's no explicit test forfilter=post_id:'...'. This may be covered implicitly by the NQL filter tests.Consider adding an explicit test for post filtering:
it('Can filter by post_id', async function () { await dbFns.addComment({ post_id: fixtureManager.get('posts', 0).id, member_id: fixtureManager.get('members', 0).id }); await dbFns.addComment({ post_id: fixtureManager.get('posts', 1).id, member_id: fixtureManager.get('members', 0).id }); await adminApi.get('/comments/?filter=' + encodeURIComponent(`post_id:'${fixtureManager.get('posts', 0).id}'`)) .expectStatus(200) .matchBodySnapshot({ comments: [commentMatcher] }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
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!**/*.snapghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
ghost/core/core/server/api/endpoints/comments.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js(2 hunks)ghost/core/core/server/services/comments/CommentsController.js(1 hunks)ghost/core/core/server/services/comments/CommentsService.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/comments.test.js(8 hunks)ghost/core/test/e2e-api/members-comments/comments.test.js(4 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/test/e2e-api/members-comments/comments.test.js
🧰 Additional context used
🧠 Learnings (6)
📚 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/web/api/endpoints/admin/routes.jsghost/core/core/server/api/endpoints/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/**/*.test.ts : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
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
📚 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.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-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/admin/comments.test.js
🧬 Code graph analysis (2)
ghost/core/core/server/web/api/endpoints/admin/routes.js (2)
ghost/core/core/server/web/api/endpoints/content/routes.js (3)
router(12-12)mw(5-5)api(3-3)ghost/core/core/server/api/endpoints/session.js (1)
api(5-5)
ghost/core/core/server/services/comments/CommentsService.js (1)
ghost/core/core/server/services/comments/CommentsController.js (1)
includeNested(105-105)
⏰ 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). (7)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Lint
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (6)
ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
44-45: LGTM! Route ordering is correct.The new
/commentsroute is correctly placed before the/comments/:idroute to ensure Express matches the collection endpoint first. The authentication middleware follows the established pattern.ghost/core/core/server/services/comments/CommentsController.js (1)
104-114: Clean controller implementation with proper separation of concerns.The boolean parsing (
!== 'false') correctly handles query string values. The controller appropriately transforms frame options into domain parameters before delegating to the service.One observation: unlike
adminBrowse, this method doesn't call#setImpersonationContext. This appears intentional since the browse-all endpoint is for moderation purposes and doesn't need member-specific data like "liked" status.ghost/core/core/server/api/endpoints/comments.js (1)
101-120: Endpoint definition follows established patterns.The empty validation object is appropriate since no parameters are required. Reusing
browsepermission (permissions: {method: 'browse'}) correctly leverages existing authorization without duplication.ghost/core/core/server/services/comments/CommentsService.js (1)
189-210: Well-documented intentional behavior difference.The explicit decision to skip
checkEnabled()is correctly documented - admins should be able to moderate existing comments even when commenting is disabled site-wide. This differs fromgetAdminComments(Line 212-217) which still checks if comments are enabled.The
parentIdlogic is correct:undefinedreturns all comments,nullfilters to top-level only.ghost/core/test/e2e-api/admin/comments.test.js (2)
12-24: Matcher updated correctly to includeparent_id.The addition of
parent_id: nullable(anyObjectId)aligns with the serializer changes and correctly usesnullable()since top-level comments havenullparent_id.
454-476: Tombstone shape is context-appropriate for per-post browse.The tombstone matcher for per-post browse correctly omits the
postrelation since the endpoint is already post-scoped. The Browse All tombstone (Lines 1333-1340) correctly includespostsince comments span multiple posts.
ref https://linear.app/ghost/issue/FEA-485 Adds GET /api/admin/comments/ endpoint for browsing all comments across posts, supporting filtering, pagination, and include_nested parameter.
rob-ghost
force-pushed
the
rob/comments-browse-all-api
branch
from
30d820f to
d6660f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
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!**/*.snapghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
ghost/core/core/server/api/endpoints/comments.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js(2 hunks)ghost/core/core/server/services/comments/CommentsController.js(1 hunks)ghost/core/core/server/services/comments/CommentsService.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/comments.test.js(8 hunks)ghost/core/test/e2e-api/members-comments/comments.test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/core/server/services/comments/CommentsController.js
- ghost/core/core/server/api/endpoints/comments.js
- ghost/core/core/server/services/comments/CommentsService.js
- ghost/core/core/server/web/api/endpoints/admin/routes.js
🧰 Additional context used
🧠 Learnings (6)
📚 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/members-comments/comments.test.jsghost/core/test/e2e-api/admin/comments.test.jsghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.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 : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
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
📚 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.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-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/admin/comments.test.js
🧬 Code graph analysis (2)
ghost/core/test/e2e-api/members-comments/comments.test.js (2)
ghost/core/test/e2e-api/admin/comments.test.js (4)
tombstoneMatcher(455-457)tombstoneMatcher(639-641)tombstoneMatcher(1333-1340)commentMatcher(1221-1235)ghost/core/core/server/api/endpoints/comments.js (4)
result(63-66)result(97-97)result(117-117)result(157-172)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js (1)
ghost/core/core/server/services/comments/CommentsController.js (1)
_(1-1)
🔇 Additional comments (12)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js (2)
51-68: Well-implemented tombstone handling for deleted comments.The early return pattern correctly short-circuits the standard mapping flow for deleted comments. The tombstone contains the minimal required information (id, parent_id, status) while preserving the post relation and recursively mapping any replies.
One consideration: the recursive
commentMapper(reply, frame)call will correctly handle deleted replies within a deleted parent, returning tombstones for those as well.
6-15: LGTM - parent_id now included in serialized output.Adding
parent_idtocommentFieldsenables the flat list representation for the new admin browse endpoint, allowing the UI to reconstruct the comment hierarchy.ghost/core/test/e2e-api/members-comments/comments.test.js (3)
123-134: Tombstone matchers align with mapper output.The
tombstoneMatchercorrectly reflects the minimal fields returned for deleted comments (id, parent_id). ThetombstoneMatcherWithReplieshelper properly expects fullcommentMatcherobjects for replies, since published replies under deleted parents should have complete data.
680-686: Test correctly updated for tombstone semantics.The test now properly verifies that a deleted parent with published replies is returned as a tombstone (status='deleted') with nested replies visible.
104-116: LGTM - commentMatcher updated with parent_id.The addition of
parent_id: nullable(anyObjectId)correctly matches the new serialization behavior where top-level comments haveparent_id: nulland replies have the parent's ObjectId.ghost/core/test/e2e-api/admin/comments.test.js (7)
1219-1235: Well-structured commentMatcher for Browse All endpoint.The matcher correctly includes:
parent_id: nullable(anyObjectId)for flat list representationedited_at: nullable(anyISODateTime)for edit trackingpostrelation (always included per PR requirements)This aligns with the PR objective that the browse-all endpoint "always includes member and post relations."
1237-1254: Good test for cross-post comment browsing.This test verifies the core functionality of the new endpoint: browsing comments across multiple posts in a single request.
1271-1284: Good test for include_nested=false behavior.This test verifies that the
include_nested=falsequery parameter correctly filters out replies, returning only top-level comments for efficient moderation views.
1372-1401: LGTM - Pagination and ordering tests.Tests properly verify:
- Pagination with
limitandpageparameters- Default ordering by
created_at desc(newest first)- Explicit ordering with
created_at asc
1350-1370: LGTM - Filter tests cover key use cases.Filtering by
statusenables moderators to find hidden comments, and filtering bymember_idallows viewing all comments from a specific member.
428-440: LGTM - Migrated to snapshot-based assertions.The tests now use
.matchBodySnapshot()for consistent validation, which improves maintainability and catches unexpected response changes.
9-24: LGTM - Updated imports and matcher.The additions of
anyStringandnullablematchers, along withparent_idinmembersCommentMatcher, properly support the new tombstone and flat-list functionality.
| // Tombstone - just id, parent_id, status (per-post browse doesn't load post relation) | ||
| const tombstoneMatcher = { | ||
| id: anyObjectId | ||
| }; | ||
|
|
||
| const deletedComment = res.body.comments[0]; | ||
| assert.equal(deletedComment.html, 'Comment 1'); | ||
| // Reply has full content including member | ||
| const replyMatcher = { | ||
| id: anyObjectId, | ||
| parent_id: anyObjectId, | ||
| created_at: anyISODateTime, | ||
| member: { | ||
| id: anyObjectId, | ||
| uuid: anyUuid | ||
| } | ||
| }; | ||
|
|
||
| const publishedReply = res.body.comments[0].replies[0]; | ||
| assert.equal(publishedReply.html, 'Reply 1'); | ||
| // Parent is tombstone with nested reply that has full content | ||
| await adminApi.get('/comments/post/' + postId + '/') | ||
| .expectStatus(200) | ||
| .matchBodySnapshot({ | ||
| comments: [{...tombstoneMatcher, replies: [replyMatcher]}] | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the comments.js file that contains the mapper
find . -name "comments.js" -type f | head -20Repository: TryGhost/Ghost
Length of output: 390
🏁 Script executed:
# Read the mapper file to see the tombstone implementation
cat -n ./ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.js | head -100Repository: TryGhost/Ghost
Length of output: 3536
🏁 Script executed:
# Read the test file around lines 454-476 and beyond to see the full test context
sed -n '440,500p' ./ghost/core/test/e2e-api/admin/comments.test.jsRepository: TryGhost/Ghost
Length of output: 2311
🏁 Script executed:
# Search for other tombstoneMatcher definitions in the test file
rg -n "tombstoneMatcher" ./ghost/core/test/e2e-api/admin/comments.test.js -A 5Repository: TryGhost/Ghost
Length of output: 1510
🏁 Script executed:
# Search for matchBodySnapshot implementation or usage patterns
rg -n "matchBodySnapshot" ./ghost/core/test/e2e-api/admin/comments.test.js | head -20Repository: TryGhost/Ghost
Length of output: 880
🏁 Script executed:
# Search for matchBodySnapshot definition
rg -n "matchBodySnapshot" ./ghost/core/test/e2e-api/ -l | head -5Repository: TryGhost/Ghost
Length of output: 319
🏁 Script executed:
# Look for the test setup and where matchBodySnapshot is defined
find ./ghost/core/test -name "*.js" -path "*/e2e-api/*" | xargs grep -l "matchBodySnapshot\s*=" | head -5Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Look for the test setup file in e2e-api
ls -la ./ghost/core/test/e2e-api/ | head -20Repository: TryGhost/Ghost
Length of output: 471
🏁 Script executed:
# Search for adminApi definition
rg -n "const adminApi" ./ghost/core/test/e2e-api/admin/comments.test.js | head -5Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Look at the top of the test file to see imports and setup
head -50 ./ghost/core/test/e2e-api/admin/comments.test.jsRepository: TryGhost/Ghost
Length of output: 1436
🏁 Script executed:
# Look at the e2e-framework to understand how matchBodySnapshot works
find ./ghost/core/test/utils -name "*.js" -type f | xargs grep -l "matchBodySnapshot" | head -5Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Find the e2e-framework setup file
find ./ghost/core/test/utils -name "*framework*" -o -name "*agent*" | head -10Repository: TryGhost/Ghost
Length of output: 452
🏁 Script executed:
# Search for matchBodySnapshot in test-agent.js
grep -n "matchBodySnapshot" ./ghost/core/test/utils/agents/test-agent.js -A 30 | head -50Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search for matchBodySnapshot in admin-api-test-agent
grep -n "matchBodySnapshot" ./ghost/core/test/utils/agents/admin-api-test-agent.js -B 5 -A 30Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search more broadly for matchBodySnapshot in all test utilities
rg "matchBodySnapshot" ./ghost/core/test/utils/ -B 3 -A 10 | head -80Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Look for snapshot files for the comments test
find ./ghost/core/test -name "*comments*" -path "*snapshot*" -o -path "*__snapshots__*" | head -10Repository: TryGhost/Ghost
Length of output: 735
Add parent_id to tombstoneMatcher to match mapper output.
The tombstoneMatcher on lines 455-457 is incomplete. The mapper always includes parent_id in deleted comments (see comments.js lines 53-56), returning either the actual parent_id or null. The matcher should include parent_id: nullable(anyObjectId) to fully verify the API response, consistent with how other matchers like membersCommentMatcher handle this field. The same applies to the tombstoneMatcher on lines 639-641.
🤖 Prompt for AI Agents
In ghost/core/test/e2e-api/admin/comments.test.js around lines 454-476 (and also
update the similar tombstoneMatcher at lines 639-641), the tombstoneMatcher is
missing parent_id which the mapper always includes (either actual parent_id or
null); update both tombstoneMatcher objects to include parent_id:
nullable(anyObjectId) so the tests match the mapper output and mirror the
pattern used in membersCommentMatcher.
There was a problem hiding this comment.
parent_id is always null and stored in the snapshot so it doesn't need to be in the matcher. The matcher is to handle dynamic values which can't be stored in the snapshots, like ids.
There was a problem hiding this comment.
@rob-ghost, thank you for the clarification! You're absolutely right—since parent_id is always null for these deleted comment tombstones, it's a static value that belongs in the snapshot rather than the matcher. The matcher should only handle dynamic values like IDs that change between test runs. I appreciate you explaining the distinction between what goes in matchers versus snapshots in your test framework.
🧠 Learnings used
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.
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.
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 : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
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`.
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
From my point of view, this looks more than good enough. The duplication between the admin and non-admin implementations at the service and model layers feels a bit iffy to me, but that seems to be in line with the pre-existing patterns.
The comment by the rabbit on the test seems valid. Maybe you want to address that.
I'm also curious to hear what @kevinansfield thinks. I'm only able to do a somewhat superficial review at this point without having dove deeper into the backend.
|
Thanks @jonatansberg! I agree, the duplication isn't ideal but I'm trying to carefully avoid pulling on too many threads given the speed we want to move at for this work in particular. I'd be keen to iterate on this whole backend section once we've unblocked the immediate work. The code rabbit comment is not a problem because |
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, the admin API now simply excludes deleted comments entirely. The public API continues to include deleted comments when they have replies (to preserve thread structure), but returns them with full attributes and `html: null` so the UI can show appropriate placeholder text.
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)
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)
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)
3 participants
Context
Site owners with active communities need a way to moderate comments at scale. Currently, the Admin API only supports browsing comments for a specific post (
GET /comments/post/:post_id), which works for the embedded Comments-UI but doesn't support a dedicated moderation interface where admins need to see and act on comments across their entire site.This is the backend foundation for the upcoming comment moderation feature (FEA-485), which will allow admins to browse, filter, and bulk-moderate comments from a central location in Ghost Admin.
What this PR does
Adds a new Admin API endpoint
GET /ghost/api/admin/comments/that:post_idrequired)parent_idreferences (rather than nested trees) to enable efficient pagination and future bulk operationsmemberandpostrelations (noincludeparam needed)include_nestedquery param to control whether replies appear in the flat list (default: true) or only top-level comments are returnedid,post_id,parent_id,status) when they have visible replies, so the UI can show "Reply to deleted comment" without leaking member/content dataThe existing per-post endpoint (
GET /comments/post/:post_id) is unchanged.How to test
ref https://linear.app/ghost/issue/FEA-485