Released comments threading and pinned comments#28125
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:
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 (1)
WalkthroughThis PR moves the comment-related flags Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 1m 56s | View ↗ |
nx run @tryghost/admin-x-settings:test:acceptance |
✅ Succeeded | 8m 51s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 7m 23s | View ↗ |
nx run ghost:test:ci:legacy |
✅ Succeeded | 3m 6s | View ↗ |
nx build @tryghost/sodo-search |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/activitypub |
✅ Succeeded | 2s | View ↗ |
nx build @tryghost/announcement-bar |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/portal |
✅ Succeeded | <1s | View ↗ |
Additional runs (8) |
✅ Succeeded | ... | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-05-26 14:20:41 UTC
jonatansberg
force-pushed
the
ber-3685-ga-threading-and-pinned-comments
branch
3 times, most recently
from
ad6f664 to
17bb20b
Compare
jonatansberg
force-pushed
the
ber-3685-ga-threading-and-pinned-comments
branch
from
17bb20b to
8daf285
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/shared/labs.js (1)
27-28: 🏗️ Heavy liftUpdate on GA flag promotion cleanup for
commentsThreads/commentsPinningNo migration/backfill was found to remove these keys from existing
settings.labsJSON. However, the settings write paths filterlabspayloads toWRITABLE_KEYS_ALLOWLISTbeforeSettingsModelvalidation, so existing DB values containingcommentsThreads/commentsPinningshouldn’t causeValidationErroron subsequentlabsupdates. (ghost/core/core/server/api/endpoints/utils/serializers/input/settings.jsandghost/core/core/server/data/importer/importers/data/settings-importer.jsboth drop non-allowlisted lab keys.)Optional: add a migration to strip stale GA keys from stored
settings.labsfor cleanliness/hygiene.🤖 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/shared/labs.js` around lines 27 - 28, Add a migration that cleans stale GA keys 'commentsThreads' and 'commentsPinning' from stored settings.labs JSON: load rows where key === 'labs' from SettingsModel (or the settings table), parse the JSON value, remove those keys if present, and write the cleaned JSON back (ensuring the update goes through the same validation path used by SettingsModel); reference WRITABLE_KEYS_ALLOWLIST behavior in comments and ensure the migration is idempotent and reversible (or logged) so it can be run safely during deploy.
🤖 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.
Nitpick comments:
In `@ghost/core/core/shared/labs.js`:
- Around line 27-28: Add a migration that cleans stale GA keys 'commentsThreads'
and 'commentsPinning' from stored settings.labs JSON: load rows where key ===
'labs' from SettingsModel (or the settings table), parse the JSON value, remove
those keys if present, and write the cleaned JSON back (ensuring the update goes
through the same validation path used by SettingsModel); reference
WRITABLE_KEYS_ALLOWLIST behavior in comments and ensure the migration is
idempotent and reversible (or logged) so it can be run safely during deploy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c2ddb47-44f9-4dba-85e4-dae8b1601f38
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxghost/core/core/shared/labs.js
💤 Files with no reviewable changes (1)
- apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
ref https://linear.app/ghost/issue/BER-3685/ga-threading-and-pinned-comments Readers can now follow deeper reply conversations with threaded comments available to everyone.
ref https://linear.app/ghost/issue/BER-3685/ga-threading-and-pinned-comments Staff can now pin top-level comments so important discussion stays visible for readers.
jonatansberg
force-pushed
the
ber-3685-ga-threading-and-pinned-comments
branch
from
8daf285 to
9745d25
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/tests/public/comment-replies.test.ts (2)
64-64: ⚡ Quick winConsider aligning test name with repository guideline.
The test name should follow the pattern:
'what is tested - expected outcome'in lowercase. As per coding guidelines, e2e test names in this repository should use this format for consistency.Consider renaming to something like:
'nested reply in threaded layout - hides reply-to context label'🤖 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 `@e2e/tests/public/comment-replies.test.ts` at line 64, Rename the test case currently declared as test('reply to reply comment in threaded layout', async ({page}) => ...) to follow the repository convention "what is tested - expected outcome" in lowercase; update the test name to a form like 'nested reply in threaded layout - hides reply-to context label' (or similar) so it matches the pattern and keep the rest of the test body unchanged.
64-96: ⚡ Quick winAdd assertion to verify threaded layout behavior.
The test name explicitly mentions "in threaded layout", but the test doesn't assert the key threaded-specific behavior. According to the relevant code snippets, threaded layout hides the "Replied to:" context label for direct children replies. The admin test suite explicitly verifies this with
await expect(directReply.getByText('Replied to:')).toBeHidden().Consider adding a similar assertion after line 95 to ensure the threaded layout is correctly hiding the "Replied to:" label:
🧪 Suggested assertion to validate threaded behavior
await expect(postCommentsSection.comments.last()).toContainText('My reply'); +await expect(postCommentsSection.comments.last().getByText('Replied to:')).toBeHidden();Note: Adjust the locator syntax based on your PostPage implementation.
🤖 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 `@e2e/tests/public/comment-replies.test.ts` around lines 64 - 96, Add an assertion that verifies the threaded layout hides the "Replied to:" context label for direct child replies: after using postCommentsSection.replyToComment(...) and before the final expectations, locate the direct reply element (use the PostPage/postCommentsSection helpers such as postCommentsSection.comments or a specific locator for the newly created reply) and assert that the "Replied to:" text is hidden (similar to the admin test's await expect(directReply.getByText('Replied to:')).toBeHidden()). This ensures the test named "reply to reply comment in threaded layout" actually validates the threaded-specific UI behavior.
🤖 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.
Nitpick comments:
In `@e2e/tests/public/comment-replies.test.ts`:
- Line 64: Rename the test case currently declared as test('reply to reply
comment in threaded layout', async ({page}) => ...) to follow the repository
convention "what is tested - expected outcome" in lowercase; update the test
name to a form like 'nested reply in threaded layout - hides reply-to context
label' (or similar) so it matches the pattern and keep the rest of the test body
unchanged.
- Around line 64-96: Add an assertion that verifies the threaded layout hides
the "Replied to:" context label for direct child replies: after using
postCommentsSection.replyToComment(...) and before the final expectations,
locate the direct reply element (use the PostPage/postCommentsSection helpers
such as postCommentsSection.comments or a specific locator for the newly created
reply) and assert that the "Replied to:" text is hidden (similar to the admin
test's await expect(directReply.getByText('Replied to:')).toBeHidden()). This
ensures the test named "reply to reply comment in threaded layout" actually
validates the threaded-specific UI behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fce400a4-c9d8-4b23-9b7b-a635574608da
📒 Files selected for processing (1)
e2e/tests/public/comment-replies.test.ts
ref https://linear.app/ghost/issue/BER-3685/ga-threading-and-pinned-comments Threaded comments now hide the reply-to-reply context label, so the public comments E2E should assert that layout instead of the legacy flat reply copy.
jonatansberg
force-pushed
the
ber-3685-ga-threading-and-pinned-comments
branch
from
ed9b482 to
94d2b29
Compare
2 participants
Summary
commentsThreadsby moving it from private labs to GA and removing it from the private Labs admin UI.commentsPinningby moving it from private labs to GA and removing it from the private Labs admin UI.Why
These comment features are ready to go out to everyone in the next release.
ref https://linear.app/ghost/issue/BER-3685/ga-threading-and-pinned-comments
Testing
pnpm --dir ghost/core exec eslint --cache -- core/shared/labs.jspnpm --dir apps/admin-x-settings exec eslint --cache -- src/components/settings/advanced/labs/private-features.tsxpnpm test:single test/e2e-api/admin/settings.test.js(29 passing)pnpm test:single test/unit/shared/labs.test.js(9 passing)