🐛 Fixed duplicate replies when replying after opening a comment permalink#28504
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR removes the client-side Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3d7c19827
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hasMore = false; | ||
| } | ||
| async function loadMoreReplies({state, api, data: {comment, limit}}: {state: EditableAppContext, api: GhostApi, data: {comment: Comment, limit?: number | 'all'}}): Promise<Partial<EditableAppContext>> { | ||
| const data = await api.comments.replies({commentId: comment.id, limit}); |
There was a problem hiding this comment.
When state.admin is set, this now always refetches replies through the members API instead of state.adminApi.replies. That drops the admin-only behavior used elsewhere for reply loads, including impersonate_member_uuid (see apps/comments-ui/src/utils/admin-api.ts lines 103-116, and the admin API test at ghost/core/test/e2e-api/admin/comments.test.js lines 1204-1205) and visibility of admin-only hidden/deleted replies. In the admin comments UI, opening a reply form on a thread with replies beyond the inline set can therefore replace/complete the thread with member-scoped data rather than moderator-scoped data.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/comments-ui/test/utils/mocked-api.ts (1)
619-639:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward
pagein the admin replies mock too.The public handler on Lines 531-540 now mirrors the new page-based contract, but
adminRequestHandlers.getRepliesstill never reads or passespage, so every admin reply request is effectively forced back to page 1. That leaves admin-mode pagination tests out of sync with the real endpoint and can hide the exact class of regression this PR is fixing.Proposed fix
async getReplies(route) { const failureResponse = await this.#handleFailure('getReplies'); if (failureResponse) { return route.fulfill(failureResponse); } await this.#delayResponse(); const url = new URL(route.request().url()); const limit = parseInt(url.searchParams.get('limit') ?? '5'); + const page = parseInt(url.searchParams.get('page') ?? '1'); const commentId = url.pathname.split('/').reverse()[2]; const filter = url.searchParams.get('filter') ?? ''; const memberUuid = url.searchParams.get('impersonate_member_uuid') ?? ''; await route.fulfill({ status: 200, body: JSON.stringify(this.browseReplies({ limit, + page, filter, commentId, memberUuid })) }); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/comments-ui/test/utils/mocked-api.ts` around lines 619 - 639, The admin mock handler async getReplies(route) is not forwarding the page query so admin pagination always uses page 1; parse the page (e.g., const page = parseInt(url.searchParams.get('page') ?? '1')) from the request URL inside getReplies and include it when calling this.browseReplies by adding page to the options object (so browseReplies receives { limit, filter, commentId, memberUuid, page }); update any variable names referenced accordingly to ensure adminRequestHandlers.getReplies mirrors the public handler's page-based contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/comments-ui/test/utils/mocked-api.ts`:
- Around line 619-639: The admin mock handler async getReplies(route) is not
forwarding the page query so admin pagination always uses page 1; parse the page
(e.g., const page = parseInt(url.searchParams.get('page') ?? '1')) from the
request URL inside getReplies and include it when calling this.browseReplies by
adding page to the options object (so browseReplies receives { limit, filter,
commentId, memberUuid, page }); update any variable names referenced accordingly
to ensure adminRequestHandlers.getReplies mirrors the public handler's
page-based contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc49a741-8117-4cfd-9eb5-441c409e8c2b
📒 Files selected for processing (4)
apps/comments-ui/src/actions.tsapps/comments-ui/src/utils/api.tsapps/comments-ui/test/unit/actions.test.jsapps/comments-ui/test/utils/mocked-api.ts
jonatansberg
force-pushed
the
ber-3706-duplicate-permalink-replies
branch
from
d3d7c19 to
016f531
Compare
jonatansberg
force-pushed
the
ber-3706-duplicate-permalink-replies
branch
from
016f531 to
f5c9ce1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/comments-ui/test/utils/mocked-api.ts (1)
627-639:⚠️ Potential issue | 🟠 MajorFix admin
getRepliesmock to forwardpageand stop passingmemberUuid
- The admin
getReplieshandler never parses/forwards thepagequery param, so it always returnsbrowseReplieswith the defaultpage=1, making admin-side pagination metadata incorrect.- It also passes
memberUuidintothis.browseReplies(...), butbrowseRepliesonly accepts{ commentId, filter, limit, page }, so this is an invalid argument (and can trigger TypeScript excess-property errors).Suggested fix
async getReplies(route) { const failureResponse = await this.#handleFailure('getReplies'); if (failureResponse) { return route.fulfill(failureResponse); } await this.#delayResponse(); const url = new URL(route.request().url()); const limit = parseInt(url.searchParams.get('limit') ?? '5'); + const page = parseInt(url.searchParams.get('page') ?? '1'); const commentId = url.pathname.split('/').reverse()[2]; const filter = url.searchParams.get('filter') ?? ''; - const memberUuid = url.searchParams.get('impersonate_member_uuid') ?? ''; await route.fulfill({ status: 200, body: JSON.stringify(this.browseReplies({ limit, + page, filter, - commentId, - memberUuid + commentId })) }); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/comments-ui/test/utils/mocked-api.ts` around lines 627 - 639, The admin getReplies mock is not forwarding the page query and is passing an unused memberUuid into this.browseReplies (which only accepts {commentId, filter, limit, page}); update the handler to parse page from url.searchParams (e.g., const page = parseInt(url.searchParams.get('page') ?? '1')) and pass page along to this.browseReplies, and remove memberUuid from the arguments so you call this.browseReplies({ limit, filter, commentId, page }) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/comments-ui/test/utils/mocked-api.ts`:
- Around line 627-639: The admin getReplies mock is not forwarding the page
query and is passing an unused memberUuid into this.browseReplies (which only
accepts {commentId, filter, limit, page}); update the handler to parse page from
url.searchParams (e.g., const page = parseInt(url.searchParams.get('page') ??
'1')) and pass page along to this.browseReplies, and remove memberUuid from the
arguments so you call this.browseReplies({ limit, filter, commentId, page })
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9b04ad1-d99d-46b2-9415-a6f365f4dd74
📒 Files selected for processing (3)
apps/comments-ui/src/actions.tsapps/comments-ui/src/utils/api.tsapps/comments-ui/test/utils/mocked-api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/comments-ui/src/utils/api.ts
…link ref https://linear.app/ghost/issue/BER-3706 - Opening a reply form re-fetched and re-appended the parent comment's replies. The comments API now returns every reply for a top-level comment inline, so state already held the full set and this re-fetch was redundant — and it duplicated replies whenever created_at order diverged from id order (the old `id:>` reply cursor re-returned already-loaded replies). It was most visible after a permalink bulk-loads all of a comment's replies and the member then clicks Reply. - Removed the redundant reply preload from openCommentForm and the now-unused loadMoreReplies action. - Switched the remaining bulk reply load (api.comments.replies with limit:'all', used by addReply) from an `id:>` cursor to page-based pagination so it stays correct for threads with >100 replies and does not rely on limit=all (which Ghost 6 clamps to 100).
jonatansberg
force-pushed
the
ber-3706-duplicate-permalink-replies
branch
from
f5c9ce1 to
bef6807
Compare
1 participant
ref BER-3706
Problem
When a member opens a comment permalink (which bulk-loads all of a parent comment's replies) and then clicks Reply, the replies render twice.
It reproduces only when a comment's replies have a
created_atorder that diverges from theiridorder (e.g. imported/backdated/seeded data) — which is why it shows on dev/reset:datadata but is rare in production. With clean fixtures (created_atmonotonic withid) it does not reproduce.Root cause
Opening a reply form ran
loadMoreReplies, which re-fetched the parent comment's replies and appended them to the ones already in state, paginating with anid:>'<lastLoadedId>'cursor. But the API returns replies ordered bycreated_at ASC, id ASC, so when those orders diverge the cursor hands back replies that are already loaded; appended without de-duplication, they render twice (key={reply.id}).The deeper issue:
loadMoreRepliesis dead weight. Since #26755 the comments API returns every reply for a top-level comment inline (the old "limit 3" was removed), and #28052 made reply "show more" purely client-side. So by the time a reply form opens, state already holds the full reply set — the re-fetch is redundant and was the sole source of the duplication.Fix
openCommentFormand the now-unusedloadMoreRepliesaction. This fixes the duplication at its source (no re-fetch → no overlap) and drops a network round-trip on every Reply click.api.comments.replies({limit: 'all'}), still used byaddReply) from theid:>cursor to page-based pagination (limit=100&page=N). This keeps it correct for threads with >100 replies (where the old cursor could skip/duplicate) and avoids relying onlimit=all, which Ghost 6 clamps to 100.mocked-api.ts) now honorspage, matching the real endpoint.Net diff is mostly deletion (3 files, +27 / −83).
Behaviour note
Previously the form-open re-fetch could surface a reply another member posted since page load the moment you opened the form. Now such a reply appears when you submit (via
addReply, which already refetches) — negligible since comments aren't live-updating, and arguably cleaner (the thread no longer grows when you click Reply).Testing
pnpm --filter @tryghost/comments-ui exec vitest run→ 259 passing.eslint→ 0 errors.Out of scope
A separate, pre-existing
addReplystale-cache edge case (rapidly posting a second reply can drop an earlier just-posted one) was identified but intentionally not addressed here, as fixing it correctly touches reply-count/tombstone semantics. Tracked separately.