Skip to content

Fix comments UI nullable reply HTML handling#28938

Merged
9larsons merged 5 commits into
TryGhost:mainfrom
PedroMarianoAlmeida:fix-comments-ui-comment-types
Jul 1, 2026
Merged

Fix comments UI nullable reply HTML handling#28938
9larsons merged 5 commits into
TryGhost:mainfrom
PedroMarianoAlmeida:fix-comments-ui-comment-types

Conversation

@PedroMarianoAlmeida

@PedroMarianoAlmeida PedroMarianoAlmeida commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Why I am making it

This started as a comments-ui typecheck cleanup. Comment.html is typed as string | null, because deleted comment replies can be returned with html: null when they need to stay visible as tombstones for visible descendant replies.

Some reply paths still treated comment.html as always being a string when rendering or building the “reply to” snippet. This makes those paths narrow nullable HTML explicitly and adds regression coverage for the deleted reply tombstone case.

What it does

  • Narrows nullable comment HTML before rendering comment content.
  • Avoids generating a reply snippet from a reply when its HTML is null.
  • Only renders ReplyFormBox when an openForm exists.
  • Adds a comments-ui E2E regression test for a deleted reply with html: null and a visible child reply.
  • Verifies the deleted reply renders as a tombstone without comment content, reply action, or like action.

Type errors fixed

This removes the apps/comments-ui/src/components/content/comment.tsx errors caused by nullable comment HTML and optional reply form state:

  • Argument of type 'Comment' is not assignable to parameter of type '{ html?: string | undefined; }'
  • Type 'string | null' is not assignable to type 'string | undefined'
  • Type 'OpenCommentForm | undefined' is not assignable to type 'OpenCommentForm'
  • Type 'string | null' is not assignable to type 'string'

Why developers need it

This cleans up the comments-ui typecheck path and protects the threaded comments UI from crashing or exposing actions for deleted reply tombstones when API data contains html: null.

Testing

  • rtk pnpm --filter @tryghost/comments-ui test:acceptance test/e2e/content.test.ts
  • rtk pnpm --dir apps/comments-ui exec eslint --cache -- test/e2e/content.test.ts
    • Exits 0 with the existing warning that the test file is ignored because comments-ui lint config only targets src.

Checklist

  • I've read and followed the Contributor Guide
  • I've explained my change
  • I've written an automated test to prove my change works

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 122aac7b-e831-46d8-9951-7c0668198e70

📥 Commits

Reviewing files that changed from the base of the PR and between 2eca515 and 497d600.

📒 Files selected for processing (1)
  • apps/comments-ui/src/components/content/comment.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/comments-ui/src/components/content/comment.tsx

Walkthrough

Comment rendering now treats missing HTML as absent content: published and unpublished layouts only show ReplyFormBox when the reply form is visible, CommentBody returns null when html is missing, and getCommentInReplyToSnippet treats null HTML as empty text. The unit test helper and snippet tests were updated for nullable HTML, and the e2e suite adds coverage for a deleted reply with null HTML rendering as a tombstone while a nested visible child reply remains visible.

Suggested reviewers: 9larsons

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: handling nullable reply HTML in the comments UI.
Description check ✅ Passed The description matches the implemented nullable HTML and tombstone rendering fixes, plus the added regression test.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Guard nullable comment HTML before rendering bodies or generating reply snippets, and only pass concrete open reply forms to ReplyFormBox.
@9larsons 9larsons force-pushed the fix-comments-ui-comment-types branch from a76d5e0 to 9ae40d9 Compare June 30, 2026 21:30
@9larsons

9larsons commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Thanks @PedroMarianoAlmeida - I changed a few of the patterns and did a little bit of cleanup along the way, but the underlying changes were correct.

@nx-cloud

nx-cloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 9dbdf2c

Command Status Duration Result
nx run-many --target=build --projects=tag:publi... ✅ Succeeded <1s View ↗
nx run @tryghost/comments-ui:test:acceptance ✅ Succeeded 48s View ↗
nx run-many -t test:unit -p @tryghost/comments-ui ✅ Succeeded 18s View ↗
nx run @tryghost/admin:build ✅ Succeeded 5s View ↗
nx run ghost:build:assets ✅ Succeeded 2s View ↗
nx run-many -t lint -p @tryghost/comments-ui ✅ Succeeded 5s View ↗
nx run ghost:build:tsc ✅ Succeeded 6s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-07-01 13:22:52 UTC

@9larsons 9larsons enabled auto-merge (squash) July 1, 2026 13:20
@9larsons 9larsons merged commit b0c8bda into TryGhost:main Jul 1, 2026
38 checks passed
9larsons pushed a commit that referenced this pull request Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants