Skip to content

🐛 Fixed reply form showing parent comment author's details#4

Open
tomerqodo wants to merge 2 commits intoclaude_claude_vs_qodo_base__fixed_reply_form_showing_parent_comment_authors_details_pr2from
claude_claude_vs_qodo_head__fixed_reply_form_showing_parent_comment_authors_details_pr2
Open

🐛 Fixed reply form showing parent comment author's details#4
tomerqodo wants to merge 2 commits intoclaude_claude_vs_qodo_base__fixed_reply_form_showing_parent_comment_authors_details_pr2from
claude_claude_vs_qodo_head__fixed_reply_form_showing_parent_comment_authors_details_pr2

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#2

rob-ghost and others added 2 commits January 25, 2026 11:56
ref https://linear.app/ghost/issue/BER-3212

When a user opened a reply form, the form header was showing the parent
comment author's name and expertise instead of the current user's data.
This happened because the code used a fallback pattern that pulled from
the comment being replied to when the current member had no value set.
reply: {
post_id: postId,
in_reply_to_id: openForm.in_reply_to_id,
in_reply_to_id: parent.id,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Changing in_reply_to_id from openForm.in_reply_to_id to parent.id on line 30 breaks reply-to-reply threading. parent is always the top-level comment, so all replies will be stored as direct replies to the parent rather than to the specific reply being responded to, losing the nested reply chain and breaking the "Reply to: " UI indicator.

Extended reasoning...

What the bug is

In reply-form.tsx line 30, the PR changes in_reply_to_id: openForm.in_reply_to_id to in_reply_to_id: parent.id. The parent prop is always the top-level comment in a thread, not the specific reply being responded to. This means all replies—whether to the top-level comment or to a nested reply—will be sent to the server with the same in_reply_to_id (the top-level comment ID), destroying the reply-to-reply relationship.

The specific code path

When a user clicks "Reply" on a nested reply, the flow is:

  1. comment.tsx line 109-111: If replying to a reply (parent exists), openReplyForm sets openForm.in_reply_to_id = comment.id (the specific reply’s ID) and openForm.in_reply_to_snippet to that reply’s text.
  2. ReplyFormBox (comment.tsx line 251) passes parent={comment} where comment is the top-level comment.
  3. The old code correctly used openForm.in_reply_to_id to send the specific reply’s ID to the server.
  4. The new code uses parent.id, which is always the top-level comment’s ID.

Step-by-step proof

Consider a top-level comment with ID "1" and a reply with ID "2". A user clicks Reply on reply "2":

  • openReplyForm sets openForm.in_reply_to_id = "2" and openForm.in_reply_to_snippet = "This is a reply to 1"
  • ReplyFormBox passes parent = the top-level comment (ID "1")
  • Old code: in_reply_to_id: openForm.in_reply_to_id → sends "2" to server ✔️
  • New code: in_reply_to_id: parent.id → sends "1" to server ❌

The server stores the reply as a direct response to comment "1" instead of reply "2".

Impact

  1. Lost threading: All replies appear as direct replies to the top-level comment. The reply-to-reply chain is lost in the database.
  2. Broken UI indicator: The RepliedToSnippet component (comment.tsx line 270-310) uses the stored in_reply_to_id to find and display the replied-to comment’s snippet. With the wrong ID, it will look up the top-level comment instead of the specific reply, showing the wrong snippet or none at all.
  3. For direct replies to parent: openForm.in_reply_to_id was previously undefined (correct—no reply-to-reply), but now it’s set to the parent’s own ID, which is semantically incorrect.
  4. Test breakage: The existing e2e test Can reply to a reply checks comment-in-reply-to text matches "This is a reply to 1", which depends on the correct in_reply_to_id being sent.

How to fix

Revert line 30 to use openForm.in_reply_to_id instead of parent.id, and restore openForm to the dependency array on line 35. The openForm.in_reply_to_id value is intentionally set by the reply-opening logic to track the specific comment being replied to.

const isReplyToReply = !!openForm.in_reply_to_snippet;
openStyles = isReplyToReply ? 'pl-[1px] pt-[68px] sm:pl-[44px] sm:pt-[56px]' : 'pl-[1px] pt-[48px] sm:pl-[44px] sm:pt-[40px]';
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Removing optional chaining on openForm (line 316: openForm.in_reply_to_snippet instead of openForm?.in_reply_to_snippet) causes a TypeError crash when the main comment form is open. main-form.tsx line 98 calls <FormWrapper editor={editor} isOpen={isOpen} reduced={false}> without passing openForm, so openForm is undefined when isOpen is true. Restore the ?. operator.

Extended reasoning...

What the bug is

The PR changed !!openForm?.in_reply_to_snippet to !!openForm.in_reply_to_snippet on line 316 of form.tsx, removing the optional chaining operator. The openForm prop is typed as optional in FormWrapperProps: openForm?: OpenCommentForm.

How it manifests

In main-form.tsx line 98, FormWrapper is called without passing openForm:

<FormWrapper editor={editor} isOpen={isOpen} reduced={false}>

This means openForm is undefined in the main comment form context.

The crash path

When isOpen is true (which happens when the user clicks or focuses the main comment editor, or when the editor has content — see main-form.tsx line 94), execution enters the if (isOpen) block at line 315. Line 316 then evaluates openForm.in_reply_to_snippet, attempting to access a property on undefined. This throws:

TypeError: Cannot read properties of undefined (reading "in_reply_to_snippet")

This crashes the entire FormWrapper component, making the main comment form completely unusable.

Step-by-step proof

  1. User navigates to a page with the Ghost comments widget.
  2. The main comment form renders via MainForm component.
  3. MainForm renders <FormWrapper editor={editor} isOpen={isOpen} reduced={false}> — no openForm prop.
  4. User clicks on or focuses the comment editor, setting isOpen = true.
  5. Inside FormWrapper, execution reaches if (isOpen) (line 315) — this is true.
  6. Line 316: const isReplyToReply = !!openForm.in_reply_to_snippet;openForm is undefined.
  7. TypeError is thrown, crashing the component.

Impact

This is a critical regression. The main comment form — the primary way users leave comments — will crash as soon as a user interacts with it. Only the reply form (which always passes openForm) would work.

Fix

Restore the optional chaining operator:

const isReplyToReply = !!openForm?.in_reply_to_snippet;

This safely returns undefined (which is falsy) when openForm is not provided, preserving the original behavior.

// Disable editing if the member doesn't have a name or when we are submitting the form
editor.setEditable(!!memberName && progress !== 'sending');
}, [editor, memberName, progress]);
editor.setEditable(!!member?.expertise && progress !== 'sending');
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: editor.setEditable on line 264 checks member?.expertise instead of memberName (i.e. member?.name). The comment on line 263 confirms the intent is to disable editing when the member has no name, but now any member with a name but no expertise gets a read-only editor, breaking the core commenting flow since expertise is optional.

Extended reasoning...

What the bug is

The useEffect in the Form component (line 258-265) controls whether the TipTap editor is editable. The original code was:

editor.setEditable(!!memberName && progress !== 'sending');

The PR changed this to:

editor.setEditable(!!member?.expertise && progress !== 'sending');

The inline comment on line 263 still reads // Disable editing if the member doesn't have a name or when we are submitting the form, confirming the original intent was to gate editability on the member having a name, not expertise.

How it manifests

Expertise is an optional field — most members will have a name but no expertise set. For these members, member?.expertise evaluates to null or undefined, so !!member?.expertise is false, and editor.setEditable(false) is called. The editor becomes read-only and the user cannot type anything.

Step-by-step proof

  1. A logged-in member has name: "John Doe" and expertise: null (the common case).
  2. The Form component mounts and runs the useEffect on line 258.
  3. It evaluates !!member?.expertise!!nullfalse.
  4. It calls editor.setEditable(false && progress !== 'sending')editor.setEditable(false).
  5. The editor is now read-only. The user sees the comment box but cannot type.
  6. Meanwhile, focusEditor in FormWrapper (line 354) still checks memberName (i.e. member?.name), so it tries to focus the editor. The editor gets focused but is not editable — a confusing broken state.

Impact

This is a functional regression that breaks the primary commenting flow for any authenticated member who hasn't set their expertise (which is the majority of users, since expertise is optional). They will see the comment form but be unable to type, with no indication of why.

How to fix

Revert line 264 to check memberName instead of member?.expertise:

editor.setEditable(!!memberName && progress !== 'sending');

And update the dependency array back to [editor, memberName, progress].

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