🐛 Fixed reply form showing parent comment author's details#26
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 member information derivation in comment forms by 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: 2
🤖 Fix all issues with AI agents
In `@apps/comments-ui/src/components/content/forms/form.tsx`:
- Around line 263-265: The gating for editable state uses member.expertise
causing users with a name but no expertise to be blocked; change the condition
in the effect that calls editor.setEditable(...) to check the member's name
(e.g., member?.name) and the sending progress instead of member?.expertise (or
restore the prior name-based gating around editor.setEditable and update tests
if expertise should be enforced elsewhere), referencing the editor.setEditable
call and the progress/member values in the effect.
- Around line 312-314: The code reads openForm.in_reply_to_snippet without
guarding for openForm being undefined, causing crashes when isOpen is true but
openForm is absent; update the condition in the component that sets openStyles
to use optional chaining (e.g., check !!openForm?.in_reply_to_snippet) or
otherwise guard openForm before accessing its in_reply_to_snippet property so
openStyles is computed safely when openForm is undefined.
📜 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 (1)
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-245: Good: memberName now sourced from the logged‑in member only.This aligns the reply form header with the current user and avoids inheriting parent comment metadata.
308-309: LGTM: member metadata derived directly from current member.This keeps the header aligned with the logged‑in user regardless of parent comment data.
apps/comments-ui/src/components/content/forms/reply-form.tsx (2)
24-35: LGTM: reply payload now targets the parent id and deps are trimmed.Using
parent.idhere is clearer and avoids reliance on openForm state.
46-49: LGTM: FormWrapper usage updated to new props.This matches the updated FormWrapper signature and keeps the reply layout intact.
apps/comments-ui/test/e2e/actions.test.ts (1)
614-680: Nice coverage for the reply‑form inheritance regression.These tests clearly capture the bug and guard against future regressions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // 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'); | ||
| }, [editor, member, progress]); |
There was a problem hiding this comment.
Editing now requires expertise — likely regression.
setEditable is now gated by member.expertise, which blocks typing for users who have a name but no expertise. The inline comment still mentions “name”. If expertise is optional, this will prevent replying. Consider reverting to name-based gating (or update behavior/tests if expertise is now required).
💡 Suggested fix
- 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.
| // 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'); | |
| }, [editor, member, progress]); | |
| // Disable editing if the member doesn't have a name or when we are submitting the form | |
| 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 263 -
265, The gating for editable state uses member.expertise causing users with a
name but no expertise to be blocked; change the condition in the effect that
calls editor.setEditable(...) to check the member's name (e.g., member?.name)
and the sending progress instead of member?.expertise (or restore the prior
name-based gating around editor.setEditable and update tests if expertise should
be enforced elsewhere), referencing the editor.setEditable call and the
progress/member values in the effect.
| 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 around the mentioned lines to understand the context
wc -l apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 142
🏁 Script executed:
# Read the context around lines 312-314
sed -n '300,330p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 989
🏁 Script executed:
# Search for the openForm variable declaration and its type
rg -B5 -A10 "const isReplyToReply = !!openForm" apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 704
🏁 Script executed:
# Search for openForm declaration and type definition in the file
rg "openForm" apps/comments-ui/src/components/content/forms/form.tsx | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 954
🏁 Script executed:
# Search for the component definition and how openForm and isOpen are used together
rg -B20 "if \(isOpen\)" apps/comments-ui/src/components/content/forms/form.tsx | head -50Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 526
🏁 Script executed:
# Check if there are other places where both isOpen and openForm are guarded
rg "if.*isOpen.*openForm|if.*openForm.*isOpen" apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Search for the component type/interface definition
rg "interface|type.*Props|FC<" apps/comments-ui/src/components/content/forms/form.tsx | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 549
🏁 Script executed:
# Find where this component is called to see if isOpen and openForm are set together
fd -e "tsx" -e "ts" | xargs rg -l "FormEditor\|Form\s*=" | head -10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Check the full FormWrapper component to see if there's any correlation between isOpen and openForm
sed -n '295,360p' apps/comments-ui/src/components/content/forms/form.tsxRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1784
🏁 Script executed:
# Search for where FormWrapper is instantiated to see if isOpen and openForm are always set together
rg "FormWrapper" apps/comments-ui/src/components/content/forms/ -A5 -B5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 6998
Guard openForm access to avoid runtime crashes.
openForm is optional and not guaranteed to be defined when isOpen is true. In main-form.tsx, the component passes isOpen without openForm, which causes a runtime error when line 313 attempts to access openForm.in_reply_to_snippet. Use optional chaining to safely access the property.
Suggested 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.
| 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]'; | |
| 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 312 -
314, The code reads openForm.in_reply_to_snippet without guarding for openForm
being undefined, causing crashes when isOpen is true but openForm is absent;
update the condition in the component that sets openStyles to use optional
chaining (e.g., check !!openForm?.in_reply_to_snippet) or otherwise guard
openForm before accessing its in_reply_to_snippet property so openStyles is
computed safely when openForm is undefined.
2 participants
Benchmark PR from qodo-benchmark#220
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.