🐛 Fixed reply form showing parent comment author's details#13
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 FormWrapper to remove the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🤖 Fix all issues with AI agents
In `@apps/comments-ui/src/components/content/forms/form.tsx`:
- Around line 264-265: The editability check currently gates editor.setEditable
on member?.expertise which will prevent users without expertise from typing;
verify whether editability should depend on member.name (as the comment implied)
or on expertise; then update the condition and dependency array in the effect
accordingly (e.g., use !!member?.name && progress !== 'sending' if name is
required, or remove the expertise check entirely if expertise is optional) and
update the nearby comment to reflect the chosen gating rule; ensure you
reference editor.setEditable, member, and progress when making the change.
- Around line 311-314: The code reads openForm.in_reply_to_snippet when
computing openStyles even though openForm is optional in FormWrapperProps and
MainForm can set isOpen without openForm; guard that access by checking openForm
exists (e.g., use optional chaining or an explicit null check) before reading
in_reply_to_snippet in the block that sets openStyles (the isOpen branch), so
replace the direct access with a safe check (references: openForm,
in_reply_to_snippet, isOpen, openStyles, FormWrapperProps, MainForm).
📜 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 (1)
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 (5)
apps/comments-ui/src/components/content/forms/form.tsx (2)
244-244: Member name now sourced solely frommember— good alignment with the fix.
308-310: Member name/expertise now derive only from current member — consistent with the fix.apps/comments-ui/src/components/content/forms/reply-form.tsx (2)
24-35:in_reply_to_idnow usesparent.idand deps updated — solid fix.
47-48: FormWrapper usage matches new prop API.apps/comments-ui/test/e2e/actions.test.ts (1)
614-680: Great regression coverage for reply-form identity behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| editor.setEditable(!!member?.expertise && progress !== 'sending'); | ||
| }, [editor, member, progress]); |
There was a problem hiding this comment.
Verify editor editability gating on expertise (could block users without expertise).
This now disables editing for members who haven’t set expertise. If expertise is optional, users may be unable to type. Also the comment above mentions “name,” so either the logic or comment is out of sync. Please confirm the intended requirement.
🔧 Suggested adjustment (if editability should depend on name as before)
- editor.setEditable(!!member?.expertise && progress !== 'sending');
+ editor.setEditable(!!member?.name && progress !== 'sending');📝 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.
| editor.setEditable(!!member?.expertise && progress !== 'sending'); | |
| }, [editor, member, progress]); | |
| editor.setEditable(!!member?.name && progress !== 'sending'); | |
| }, [editor, member, progress]); |
🤖 Prompt for AI Agents
In `@apps/comments-ui/src/components/content/forms/form.tsx` around lines 264 -
265, The editability check currently gates editor.setEditable on
member?.expertise which will prevent users without expertise from typing; verify
whether editability should depend on member.name (as the comment implied) or on
expertise; then update the condition and dependency array in the effect
accordingly (e.g., use !!member?.name && progress !== 'sending' if name is
required, or remove the expertise check entirely if expertise is optional) and
update the nearby comment to reflect the chosen gating rule; ensure you
reference editor.setEditable, member, and progress when making the change.
| 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 file to understand the context around lines 311-314
wc -l apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 142
🏁 Script executed:
# Read the relevant section and surrounding context
sed -n '290,330p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1204
🏁 Script executed:
# Find the component's prop definition to check if openForm is optional
rg -n "interface.*Props|type.*Props|openForm.*:" apps/comments-ui/src/components/content/forms/form.tsx -A 20 -B 2Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 3881
🏁 Script executed:
# Find all FormWrapper usages to verify the issue
rg -n "FormWrapper" apps/comments-ui/src --type=tsx -B 2 -A 5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 111
🏁 Script executed:
# Also check if OpenCommentForm type definition shows required vs optional fields
rg -n "OpenCommentForm" apps/comments-ui/src --type=tsx -B 2 -A 10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 111
🏁 Script executed:
# Find all FormWrapper usages
rg -n "FormWrapper" apps/comments-ui/src -B 2 -A 5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 6049
🏁 Script executed:
# Find OpenCommentForm type definition
rg -n "OpenCommentForm" apps/comments-ui/src -B 2 -A 10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 17498
🏁 Script executed:
# Let's examine the main-form.tsx file more closely to understand the isOpen state
cat -n apps/comments-ui/src/components/content/forms/main-form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 4629
Guard optional openForm access to avoid runtime crashes.
openForm is optional in FormWrapperProps, but line 313 accesses openForm.in_reply_to_snippet without a null check. MainForm (line 98) passes isOpen={true} without providing openForm, which will crash at runtime when the code attempts to read the property.
Proposed 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.
| 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]'; | |
| 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 311 -
314, The code reads openForm.in_reply_to_snippet when computing openStyles even
though openForm is optional in FormWrapperProps and MainForm can set isOpen
without openForm; guard that access by checking openForm exists (e.g., use
optional chaining or an explicit null check) before reading in_reply_to_snippet
in the block that sets openStyles (the isOpen branch), so replace the direct
access with a safe check (references: openForm, in_reply_to_snippet, isOpen,
openStyles, FormWrapperProps, MainForm).
2 participants
Benchmark PR from qodo-benchmark#220
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.