Repository navigation
feat: comment threading UI with working reply button - #49
Merged
Merged
Conversation
Hub contract (built in parallel, fixed): POST .../comments accepts optional
parent_id; comment responses gain parent_id: str|null; depth is one level
(the hub flattens deeper nesting onto the grandparent); a reply's parent is
always within the same case.
PHP:
- HubClient::createComment() gains an optional ?string $parentId = null,
added to the hub payload via a new parentIdField() helper (same
omit-when-null idiom as emailField/metadataField).
- CommentsController::handleCreate() reads the optional parent_id request
param (parentIdFrom(), matching MessageController::triggerFrom()'s
is_string()-and-non-empty idiom) and forwards it through.
- Tests: HubClient payload contains/omits parent_id; CommentsController
forwards/omits it to the hub.
JS:
- topics.js: mapCommentToView maps parent_id -> parentId (defaults to null
for an older hub). New groupCommentThreads(comments): given the hub's flat,
chronological list, returns one entry per top-level comment in the
existing newest-first sort, each carrying a `replies` array in oldest-first
order. A tombstoned top-level comment still anchors its replies; an
orphaned reply (parent not in the list) is dropped rather than shown
detached.
- topic-detail.jsx: TopicDetail keeps the hub's chronological order in state
(grouping/sorting now happens at render via groupCommentThreads) and
renders each thread as its top-level CommentRow followed by its replies,
indented one level (rt-cm-reply-row). Reply state (`{handle, parentId}` or
null) lives in TopicDetail; clicking Reply on either a top-level comment or
one of its replies always targets that thread's top-level id as parent_id
(matching the hub's one-level flattening) while the "Replying to <handle>"
chip shows whichever comment was actually clicked. Exported CommentRow so
its indent/Reply-wiring is directly testable (TopicDetail's own comments
never populate through this SSR-only test harness — no fetch, no effects).
- comment-actions.jsx: CommentActions takes an onReply prop; Reply is
enabled/wired when given, the vote button stays permanently disabled
("coming in a later release", unchanged).
- comment-editor.jsx: CommentEditor renders a "Replying to <handle>" chip
with a cancel (x) above the editor when replyTarget is set; forwards a new
focusNonce to WysiwygEditor.
- wysiwyg-editor.jsx: WysiwygEditor gains a focusNonce prop (same
changing-nonce idiom as the existing resetNonce) that moves keyboard focus
into the editor — the TinyMCE instance via tinymce.get(id).focus(), or the
fallback textarea directly.
- CommentComposer (topic-detail.jsx) sends replyTarget?.parentId to
postComment (api.js gained an optional third parentId argument, omitted
from the POST body when absent); after a successful post, both the
composer text and reply mode clear together. Replying while the composer
already holds text does not touch that text — reply-mode state and
composer text are fully independent, by construction.
- Pending topics: unchanged — the composer still renders with only submit
disabled; no vote/comment fetches.
Tests (static-render style): thread grouping/indent render
(groupCommentThreads + CommentRow's rt-cm-reply-row class and Reply
enablement), reply-chip render (present/absent) in CommentEditor,
parentId/parent_id mapping defaults in topics.js and api.js. The literal
DOM-focus effect (focusNonce) isn't verifiable through renderToString (no
effects run) — implemented via the same nonce idiom as the already-shipped,
equally untestable resetNonce; not separately unit-tested.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review found the implementation correct but flagged two gaps; tests only
(plus the minimal extraction testability required).
1. The thread-wiring glue in topic-detail.jsx (the groupCommentThreads(...)
.map block building the CommentRow tree and each onReply closure) had no
test — a regression swapping thread.id/reply.id, dropping isReply, or
using the wrong handle would go uncaught, and the harness never runs
effects so it can't be caught through TopicDetail's own render either.
Extracted the row-building into a new pure topics.js helper,
buildThreadRows(comments): flattens groupCommentThreads' grouped output
into one {comment, isReply, replyTarget:{parentId, handle}} descriptor
per row, in display order. topic-detail.jsx's render now consumes it
directly (one CommentRow per row, onReply={() => startReply(row.replyTarget)}),
replacing the inline groupCommentThreads(...).map(...) block — this also
simplified the JSX (no more nested Fragment-per-thread wrapper, since
comment.id is already a unique key per flattened row). Unit-tested
directly: root rows flagged isReply:false, reply rows isReply:true, and
every row's replyTarget carries parentId = the thread's root id while
handle = that specific row's own handle (verified for both root rows,
where the target is themselves, and reply rows under the same thread).
2. Added the missing grouping edge case to the existing groupCommentThreads
test group: a reply whose parentId points at a comment that IS present in
the list but is itself a reply (not top-level). Locked in the expected
behavior — silently dropped, same outcome as the already-tested
unknown-parent orphan case, reached via a different code path (the
non-top-level id is never looked up, since only top-level ids are used as
keys when resolving each thread's replies). No production code changed
for this one — groupCommentThreads already behaved correctly; only the
test was missing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Reply button comes alive: a "Replying to " chip above the composer, replies indented one level under their thread root (oldest-first), tombstoned parents still anchoring, and parent_id flowing through the REST proxy and HubClient. Thread-row building is a pure, unit-tested helper. Reviewed; both flagged coverage gaps closed.
Tests: JS 183/183, PHP 230/230, phpcs + phpstan clean; bundle rebuilt and committed.
🤖 Generated with Claude Code