Code Review Bench PR #25858 - 🐛 Fixed reply form showing parent comment author's details#3
Conversation
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.
|
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'); |
There was a problem hiding this comment.
🚨 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:
-
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.
-
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 👍 / 👎
| 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; |
There was a problem hiding this comment.
🚨 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 👍 / 👎
| const isReplyToReply = !!openForm.in_reply_to_snippet; | |
| const isReplyToReply = !!openForm?.in_reply_to_snippet; |
- Apply suggested fix
There was a problem hiding this comment.
Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.
Code Review 🚫 Blocked 0 resolved / 2 findingsTwo critical bugs introduced: (1) 🚨 Bug: setEditable checks expertise instead of name — breaks editing📄 apps/comments-ui/src/components/content/forms/form.tsx:264 The
This appears to be an accidental change during the refactor. The line should still check the member's name. Suggested fix🚨 Bug: Removing optional chaining on
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
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:
Reference: https://linear.app/ghost/issue/BER-3212