Skip to content

Comments

Code Review Bench PR #25858 - 🐛 Fixed reply form showing parent comment author's details#3

Open
ketkarameya wants to merge 2 commits intobase_pr_25858_20260125_5844from
corrupted_pr_25858_20260125_5844
Open

Code Review Bench PR #25858 - 🐛 Fixed reply form showing parent comment author's details#3
ketkarameya wants to merge 2 commits intobase_pr_25858_20260125_5844from
corrupted_pr_25858_20260125_5844

Conversation

@ketkarameya
Copy link

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#2

Summary:
When opening a reply form on a comment, the form header was incorrectly displaying the parent comment author's name and expertise instead of the current user's data.

Fix:
Removes fallbacks so the reply form only shows the logged-in user's own details.

Changes:

  • 3 files changed
  • 78 additions, 12 deletions

Reference: https://linear.app/ghost/issue/BER-3212

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.
@ketkarameya
Copy link
Author

Gitar, Review this PR

// 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');

Choose a reason for hiding this comment

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

🚨 Bug: setEditable checks expertise instead of name — breaks editing

The setEditable condition was changed from !!memberName to !!member?.expertise, but the intent (as stated in the comment on line 263) is to "disable editing if the member doesn't have a name". This introduces two bugs:

  1. Members with a name but no expertise cannot type: The editor will be non-editable for any member who has a name set but has not filled in their expertise. This is a very common case and effectively breaks the comment form for most users.

  2. Members with expertise but no name can bypass the name check: The editor will be editable even without a name, bypassing the "force name input first" UX flow.

This appears to be an accidental change during the refactor. The line should still check the member's name.

Was this helpful? React with 👍 / 👎

Suggested change
editor.setEditable(!!member?.expertise && progress !== 'sending');
editor.setEditable(!!member?.name && progress !== 'sending');
}, [editor, member, progress]);
  • Apply suggested fix

let openStyles = '';
if (isOpen) {
const isReplyToReply = !!openForm?.in_reply_to_snippet;
const isReplyToReply = !!openForm.in_reply_to_snippet;

Choose a reason for hiding this comment

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

🚨 Bug: Removing optional chaining on openForm causes crash in main form

The change from openForm?.in_reply_to_snippet to openForm.in_reply_to_snippet (line 313) removes null safety. openForm is typed as optional (openForm?: OpenCommentForm in FormWrapperProps).

In main-form.tsx (line 98), FormWrapper is called without the openForm prop:

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

When isOpen is true (which happens whenever the editor is focused or has content, per line 94 of main-form.tsx), the code enters the if (isOpen) block and accesses openForm.in_reply_to_snippet on undefined, causing a runtime TypeError: Cannot read properties of undefined.

This means the main comment form will crash as soon as a user clicks into it or starts typing.

Was this helpful? React with 👍 / 👎

Suggested change
const isReplyToReply = !!openForm.in_reply_to_snippet;
const isReplyToReply = !!openForm?.in_reply_to_snippet;
  • Apply suggested fix

Copy link

@gitar-bot-staging gitar-bot-staging bot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR is blocked due to unresolved code review findings.

Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.

@gitar-bot-staging
Copy link

Code Review 🚫 Blocked 0 resolved / 2 findings

Two critical bugs introduced: (1) setEditable now checks expertise instead of name, breaking editor editability for most users; (2) removing optional chaining on openForm causes a runtime crash in the main comment form when it's focused.

🚨 Bug: setEditable checks expertise instead of name — breaks editing

📄 apps/comments-ui/src/components/content/forms/form.tsx:264

The setEditable condition was changed from !!memberName to !!member?.expertise, but the intent (as stated in the comment on line 263) is to "disable editing if the member doesn't have a name". This introduces two bugs:

  1. Members with a name but no expertise cannot type: The editor will be non-editable for any member who has a name set but has not filled in their expertise. This is a very common case and effectively breaks the comment form for most users.

  2. Members with expertise but no name can bypass the name check: The editor will be editable even without a name, bypassing the "force name input first" UX flow.

This appears to be an accidental change during the refactor. The line should still check the member's name.

Suggested fix
        editor.setEditable(!!member?.name && progress !== 'sending');
    }, [editor, member, progress]);
🚨 Bug: Removing optional chaining on openForm causes crash in main form

📄 apps/comments-ui/src/components/content/forms/form.tsx:313

The change from openForm?.in_reply_to_snippet to openForm.in_reply_to_snippet (line 313) removes null safety. openForm is typed as optional (openForm?: OpenCommentForm in FormWrapperProps).

In main-form.tsx (line 98), FormWrapper is called without the openForm prop:

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

When isOpen is true (which happens whenever the editor is focused or has content, per line 94 of main-form.tsx), the code enters the if (isOpen) block and accesses openForm.in_reply_to_snippet on undefined, causing a runtime TypeError: Cannot read properties of undefined.

This means the main comment form will crash as soon as a user clicks into it or starts typing.

Suggested fix
        const isReplyToReply = !!openForm?.in_reply_to_snippet;
Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

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.

3 participants