authorgravatar for 97427849+flamboh@users.noreply.github.comoliver <97427849+flamboh@users.noreply.github.com> 2026-10-14 10:49:00-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-14 17:49:00+00:00
log7897a695e351b7e8ebaa0d8c693660291af18286
tree77906fee4cf75dc8720822a90c9ba073d64cdcbc
parentce6f8ce36e240d1c615e2130fc017652cbc1ec58
signature Signed by PGP key B5690EEEBB952194

fix(web): review summary only says optional when it is (#18451)

2 files changed, 42 insertions(+), 35 deletions(-)

apps/mobile/src/features/pull-requests/PullRequestReviewSheet.tsx+1-1
...@@ -154,7 +154,7 @@ export function PullRequestReviewSheet({...@@ -154,7 +154,7 @@ export function PullRequestReviewSheet({
154 />154 />
155 }155 }
156 >156 >
157 {reviewing && offered.length ? (157 {reviewing && offered.length > 1 ? (
158 <SegmentedControl158 <SegmentedControl
159 options={offered.map((option) => ({ value: option.value, label: option.title }))}159 options={offered.map((option) => ({ value: option.value, label: option.title }))}
160 selected={verdict?.value ?? "comment"}160 selected={verdict?.value ?? "comment"}
apps/web/src/components/pullRequest/PullRequestReviewForm.tsx+41-34
...@@ -116,10 +116,17 @@ export function PullRequestReviewForm({...@@ -116,10 +116,17 @@ export function PullRequestReviewForm({
116 };116 };
117117
118 // Forgejo requires a summary when requesting changes, even with inline comments.118 // Forgejo requires a summary when requesting changes, even with inline comments.
119 const canSubmit = (verdict: PullRequestReviewVerdict) =>119 const canSubmit = (verdict: PullRequestReviewVerdict, summary: string) =>
120 verdict === "request-changes" && requestChangesSummaryRequired120 verdict === "request-changes" && requestChangesSummaryRequired
121 ? body.trim().length > 0121 ? summary.trim().length > 0
122 : verdict === "approve" || body.trim().length > 0 || comments.length > 0;122 : verdict === "approve" || summary.trim().length > 0 || comments.length > 0;
123 // The placeholder asks for exactly what the submit button is waiting on.
124 const placeholder =
125 selectedVerdict === undefined || canSubmit(selectedVerdict.value, "")
126 ? "Summarize your review (optional)"
127 : selectedVerdict.value === "request-changes" && requestChangesSummaryRequired
128 ? "Summarize your review (required to request changes)"
129 : "Summarize your review or add line comments";
123130
124 return (131 return (
125 <>132 <>
...@@ -127,42 +134,42 @@ export function PullRequestReviewForm({...@@ -127,42 +134,42 @@ export function PullRequestReviewForm({
127 ref={textareaRef}134 ref={textareaRef}
128 rows={3}135 rows={3}
129 value={body}136 value={body}
130 placeholder={137 placeholder={placeholder}
131 requestChangesSummaryRequired && verdicts.includes("request-changes")
132 ? "Summarize your review (required to request changes)"
133 : "Summarize your review (optional)"
134 }
135 aria-label="Review summary"138 aria-label="Review summary"
136 onChange={(event) => setSummary(reviewKey, event.target.value)}139 onChange={(event) => setSummary(reviewKey, event.target.value)}
137 />140 />
138 <div className="mt-2 flex justify-between gap-2">141 <div className="mt-2 flex justify-end gap-2">
139 <Select142 {offered.length > 1 ? (
140 value={selectedVerdict?.value ?? null}143 <Select
141 disabled={pending}144 value={selectedVerdict?.value ?? null}
142 onValueChange={(value) => {145 disabled={pending}
143 if (value !== null) setRequestedVerdict(value);146 onValueChange={(value) => {
144 }}147 if (value !== null) setRequestedVerdict(value);
145 >148 }}
146 <SelectTrigger size="xs" className="w-auto min-w-0" aria-label="Review verdict">149 >
147 <span className="flex items-center gap-1.5">150 <SelectTrigger size="xs" className="mr-auto w-auto min-w-0" aria-label="Review verdict">
148 {selectedVerdict?.icon}151 <span className="flex items-center gap-1.5">
149 {selectedVerdict?.label}152 {selectedVerdict?.icon}
150 </span>153 {selectedVerdict?.label}
151 </SelectTrigger>154 </span>
152 <SelectPopup side="top" alignItemWithTrigger={false}>155 </SelectTrigger>
153 {offered.map((verdict) => (156 <SelectPopup side="top" alignItemWithTrigger={false}>
154 <SelectItem key={verdict.value} value={verdict.value}>157 {offered.map((verdict) => (
155 <span className="flex items-center gap-1.5">158 <SelectItem key={verdict.value} value={verdict.value}>
156 {verdict.icon}159 <span className="flex items-center gap-1.5">
157 {verdict.label}160 {verdict.icon}
158 </span>161 {verdict.label}
159 </SelectItem>162 </span>
160 ))}163 </SelectItem>
161 </SelectPopup>164 ))}
162 </Select>165 </SelectPopup>
166 </Select>
167 ) : null}
163 <Button168 <Button
164 size="xs"169 size="xs"
165 disabled={pending || selectedVerdict === undefined || !canSubmit(selectedVerdict.value)}170 disabled={
171 pending || selectedVerdict === undefined || !canSubmit(selectedVerdict.value, body)
172 }
166 onClick={() => {173 onClick={() => {
167 if (selectedVerdict !== undefined) void submit(selectedVerdict);174 if (selectedVerdict !== undefined) void submit(selectedVerdict);
168 }}175 }}