🐛 Fixed reply form showing parent comment author's details#40
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.
WalkthroughThe changes refactor the member data source in form components, removing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/comments-ui/src/components/content/forms/reply-form.tsx (1)
24-35: The payload should preserve the intended reply-to-reply target.The change to hardcode
in_reply_to_id: parent.idbreaks reply-to-reply functionality. When a user replies to a specific reply (not the top-level comment), the form correctly populatesopenForm.in_reply_to_idwith that reply's ID and displays it to the user (via the form component). However, the submit handler ignores this and always sendsparent.id, causing the reply to be linked to the wrong comment.Use
openForm.in_reply_to_id ?? parent.idto preserve the reply-to-reply semantic.Fix
- in_reply_to_id: parent.id, + in_reply_to_id: openForm.in_reply_to_id ?? parent.id,apps/comments-ui/src/components/content/forms/form.tsx (1)
244-265: Editing is disabled for members without expertise, contradicting the intent to gate on name.The code at line 264 checks
!!member?.expertisebut the comment says "Disable editing if the member doesn't have a name". The E2E test "Can add expertise" explicitly setsexpertise: nullwhile providing a name and expects the editor to be usable. Expertise is optional (UI shows "Add your expertise"), so this check prevents commenting for users who haven't set expertise, which is unintended.🐛 Suggested fix
- // Disable editing if the member doesn't have a name or when we are submitting the form - editor.setEditable(!!member?.expertise && progress !== 'sending'); + // Disable editing if the member doesn't have a name or when we are submitting the form + editor.setEditable(!!memberName && progress !== 'sending');
🤖 Fix all issues with AI agents
In `@apps/comments-ui/src/components/content/forms/form.tsx`:
- Around line 308-314: Guard access to openForm when computing openStyles:
replace the direct property access openForm.in_reply_to_snippet with a safe
check (e.g., use optional chaining openForm?.in_reply_to_snippet or explicitly
check openForm truthiness) so that when isOpen is true but openForm is undefined
you don't throw; update the computation in the component where openStyles is set
(referencing openForm, isOpen, and openStyles in form.tsx / FormWrapper usage)
to only read in_reply_to_snippet if openForm is defined.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/comments-ui/src/components/content/forms/form.tsxapps/comments-ui/src/components/content/forms/reply-form.tsxapps/comments-ui/test/e2e/actions.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/comments-ui/src/components/content/forms/reply-form.tsx (3)
apps/comments-ui/src/components/content/comment.test.jsx (4)
parent(34-36)parent(77-79)parent(98-100)parent(115-117)apps/comments-ui/src/components/content/forms/form.tsx (1)
FormWrapper(393-393)apps/comments-ui/src/utils/helpers.ts (1)
isMobile(140-142)
apps/comments-ui/test/e2e/actions.test.ts (1)
apps/comments-ui/test/utils/e2e.ts (1)
waitEditorFocused(9-13)
🔇 Additional comments (2)
apps/comments-ui/test/e2e/actions.test.ts (1)
614-681: Solid regression coverage for reply header identity.These cases clearly assert that reply forms show the logged-in member’s data and do not inherit the parent author’s name/expertise. Nice addition.
apps/comments-ui/src/components/content/forms/reply-form.tsx (1)
47-49: LGTM: FormWrapper usage now matches the reduced props.Dropping the
commentprop and keeping the wrapper update looks consistent with the new FormWrapper API.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const memberName = member?.name; | ||
| const memberExpertise = member?.expertise; | ||
|
|
||
| let openStyles = ''; | ||
| if (isOpen) { | ||
| const isReplyToReply = !!openForm?.in_reply_to_snippet; | ||
| 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]'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the form.tsx file to understand the component props and structure
head -100 apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 3451
🏁 Script executed:
# Check the full file to find prop type definitions
wc -l apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 142
🏁 Script executed:
# Read specific sections around line 308 and look for prop definitions
sed -n '1,50p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 2090
🏁 Script executed:
# Now let's look at the specific lines mentioned in the review (308-314)
sed -n '300,320p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 671
🏁 Script executed:
# Also look for the FormWrapper component being imported and used
rg "FormWrapper" -n -B 2 -A 2Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 4097
🏁 Script executed:
# Let's check the FormWrapperProps type definition
sed -n '291,298p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 246
🏁 Script executed:
# Check the main-form.tsx to see what isOpen value is passed to FormWrapper
cat -n apps/comments-ui/src/components/content/forms/main-form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 4629
🏁 Script executed:
# Let's verify the structure around line 308 in form.tsx more carefully
sed -n '295,320p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 789
🏁 Script executed:
# Check if there are any guards or checks before the problematic code
sed -n '290,330p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1204
Fix potential undefined access: guard openForm before accessing its properties.
In main-form.tsx, FormWrapper is called with isOpen={isOpen} but without the openForm prop. Since isOpen can be true while openForm is undefined, accessing openForm.in_reply_to_snippet at line 313 will throw at runtime.
Use optional chaining to safely access the property:
Fix
- const isReplyToReply = !!openForm.in_reply_to_snippet;
+ const isReplyToReply = !!openForm?.in_reply_to_snippet;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const memberName = member?.name; | |
| const memberExpertise = member?.expertise; | |
| let openStyles = ''; | |
| if (isOpen) { | |
| const isReplyToReply = !!openForm?.in_reply_to_snippet; | |
| 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]'; | |
| const memberName = member?.name; | |
| const memberExpertise = member?.expertise; | |
| let openStyles = ''; | |
| if (isOpen) { | |
| 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]'; |
🤖 Prompt for AI Agents
In `@apps/comments-ui/src/components/content/forms/form.tsx` around lines 308 -
314, Guard access to openForm when computing openStyles: replace the direct
property access openForm.in_reply_to_snippet with a safe check (e.g., use
optional chaining openForm?.in_reply_to_snippet or explicitly check openForm
truthiness) so that when isOpen is true but openForm is undefined you don't
throw; update the computation in the component where openStyles is set
(referencing openForm, isOpen, and openStyles in form.tsx / FormWrapper usage)
to only read in_reply_to_snippet if openForm is defined.
2 participants
Benchmark PR from agentic-review-benchmarks#2
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.