feat(ui): issue review banner with approve / request changes#4435
feat(ui): issue review banner with approve / request changes#4435ramonmatias19 wants to merge 1 commit intopaperclipai:masterfrom
Conversation
Greptile SummaryThis PR adds an
Confidence Score: 3/5Not ready to merge — the rejection reason gap contradicts the stated spec, and the PR is missing required CONTRIBUTING.md sections and screenshots. One P1 finding (rejection reason described in the PR spec but not implemented in the UI or callback contract) plus two process violations (missing PR template sections including Thinking Path and Model Used, no screenshots for a visual change) bring the score to 3. The auto-reopen and banner rendering logic themselves are correct. ui/src/components/IssueChatThread.tsx — specifically the onReviewRequestChanges callback and the banner's button handler. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2759-2771
Comment:
**Missing rejection reason UI**
The PR description states that "Request changes" sets `status=in_progress` **with a rejection reason**, but the `onReviewRequestChanges` callback signature is `() => Promise<void>` — no reason string is passed, and the button immediately fires the callback without prompting the user for input. Either the callback should accept a `reason: string` parameter (and the banner should include an inline text input or open a dialog to collect it), or the PR description overstates what this action does.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2397-2398
Comment:
**Shared loading state conflates both buttons**
`reviewSubmitting` is shared, so clicking either button sets both to disabled and shows "Enviando..." on each. A user who clicks "Solicitar alterações" will also see "Enviando..." on the "Aprovar" button, which is confusing. Consider tracking which action is in-flight (e.g., `reviewSubmitting: "approve" | "request-changes" | false`) and showing the spinner only on the clicked button.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2398
Comment:
**`showReviewBanner` evaluates to a function reference, not a boolean**
`const showReviewBanner = issueStatus === "in_review" && (onReviewApprove || onReviewRequestChanges)` — when `issueStatus === "in_review"` and `onReviewApprove` is defined, this evaluates to the function itself rather than `true`. React treats a function reference as a truthy value in `{showReviewBanner && (...)}` so the component renders correctly, but TypeScript's `--strictNullChecks` may emit a warning and it silently departs from conventional boolean-flag patterns. Prefer an explicit cast:
```suggestion
const showReviewBanner = issueStatus === "in_review" && !!(onReviewApprove || onReviewRequestChanges);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2741-2780
Comment:
**Silent failure on callback error**
Both `onClick` handlers swallow errors via `finally { setReviewSubmitting(false) }`. If `onReviewApprove()` or `onReviewRequestChanges()` rejects, the button simply returns to its idle state with no feedback to the user. Consider surfacing the error (e.g., via a toast or an inline error message) so the reviewer knows the action did not complete.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(ui): issue review banner with appro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <Button | ||
| size="sm" | ||
| disabled={reviewSubmitting} | ||
| onClick={async () => { | ||
| setReviewSubmitting(true); | ||
| try { | ||
| await onReviewApprove(); | ||
| } finally { | ||
| setReviewSubmitting(false); | ||
| } | ||
| }} | ||
| > | ||
| <CheckCircle2 className="h-3.5 w-3.5 mr-1.5" /> |
There was a problem hiding this comment.
The PR description states that "Request changes" sets status=in_progress with a rejection reason, but the onReviewRequestChanges callback signature is () => Promise<void> — no reason string is passed, and the button immediately fires the callback without prompting the user for input. Either the callback should accept a reason: string parameter (and the banner should include an inline text input or open a dialog to collect it), or the PR description overstates what this action does.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2759-2771
Comment:
**Missing rejection reason UI**
The PR description states that "Request changes" sets `status=in_progress` **with a rejection reason**, but the `onReviewRequestChanges` callback signature is `() => Promise<void>` — no reason string is passed, and the button immediately fires the callback without prompting the user for input. Either the callback should accept a `reason: string` parameter (and the banner should include an inline text input or open a dialog to collect it), or the PR description overstates what this action does.
How can I resolve this? If you propose a fix, please make it concise.| const [reviewSubmitting, setReviewSubmitting] = useState(false); | ||
| const showReviewBanner = issueStatus === "in_review" && (onReviewApprove || onReviewRequestChanges); |
There was a problem hiding this comment.
reviewSubmitting is shared, so clicking either button sets both to disabled and shows "Enviando..." on each. A user who clicks "Solicitar alterações" will also see "Enviando..." on the "Aprovar" button, which is confusing. Consider tracking which action is in-flight (e.g., reviewSubmitting: "approve" | "request-changes" | false) and showing the spinner only on the clicked button.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2397-2398
Comment:
**Shared loading state conflates both buttons**
`reviewSubmitting` is shared, so clicking either button sets both to disabled and shows "Enviando..." on each. A user who clicks "Solicitar alterações" will also see "Enviando..." on the "Aprovar" button, which is confusing. Consider tracking which action is in-flight (e.g., `reviewSubmitting: "approve" | "request-changes" | false`) and showing the spinner only on the clicked button.
How can I resolve this? If you propose a fix, please make it concise.| }: IssueChatThreadProps) { | ||
| const location = useLocation(); | ||
| const [reviewSubmitting, setReviewSubmitting] = useState(false); | ||
| const showReviewBanner = issueStatus === "in_review" && (onReviewApprove || onReviewRequestChanges); |
There was a problem hiding this comment.
showReviewBanner evaluates to a function reference, not a boolean
const showReviewBanner = issueStatus === "in_review" && (onReviewApprove || onReviewRequestChanges) — when issueStatus === "in_review" and onReviewApprove is defined, this evaluates to the function itself rather than true. React treats a function reference as a truthy value in {showReviewBanner && (...)} so the component renders correctly, but TypeScript's --strictNullChecks may emit a warning and it silently departs from conventional boolean-flag patterns. Prefer an explicit cast:
| const showReviewBanner = issueStatus === "in_review" && (onReviewApprove || onReviewRequestChanges); | |
| const showReviewBanner = issueStatus === "in_review" && !!(onReviewApprove || onReviewRequestChanges); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2398
Comment:
**`showReviewBanner` evaluates to a function reference, not a boolean**
`const showReviewBanner = issueStatus === "in_review" && (onReviewApprove || onReviewRequestChanges)` — when `issueStatus === "in_review"` and `onReviewApprove` is defined, this evaluates to the function itself rather than `true`. React treats a function reference as a truthy value in `{showReviewBanner && (...)}` so the component renders correctly, but TypeScript's `--strictNullChecks` may emit a warning and it silently departs from conventional boolean-flag patterns. Prefer an explicit cast:
```suggestion
const showReviewBanner = issueStatus === "in_review" && !!(onReviewApprove || onReviewRequestChanges);
```
How can I resolve this? If you propose a fix, please make it concise.| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| disabled={reviewSubmitting} | ||
| onClick={async () => { | ||
| setReviewSubmitting(true); | ||
| try { | ||
| await onReviewRequestChanges(); | ||
| } finally { | ||
| setReviewSubmitting(false); | ||
| } | ||
| }} | ||
| > | ||
| <XCircle className="h-3.5 w-3.5 mr-1.5" /> | ||
| {reviewSubmitting ? "Enviando..." : "Solicitar alterações"} | ||
| </Button> | ||
| )} | ||
| {onReviewApprove && ( | ||
| <Button | ||
| size="sm" | ||
| disabled={reviewSubmitting} | ||
| onClick={async () => { | ||
| setReviewSubmitting(true); | ||
| try { | ||
| await onReviewApprove(); | ||
| } finally { | ||
| setReviewSubmitting(false); | ||
| } | ||
| }} | ||
| > | ||
| <CheckCircle2 className="h-3.5 w-3.5 mr-1.5" /> | ||
| {reviewSubmitting ? "Enviando..." : "Aprovar"} | ||
| </Button> | ||
| )} | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| {showComposer ? ( | ||
| <div |
There was a problem hiding this comment.
Both onClick handlers swallow errors via finally { setReviewSubmitting(false) }. If onReviewApprove() or onReviewRequestChanges() rejects, the button simply returns to its idle state with no feedback to the user. Consider surfacing the error (e.g., via a toast or an inline error message) so the reviewer knows the action did not complete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/IssueChatThread.tsx
Line: 2741-2780
Comment:
**Silent failure on callback error**
Both `onClick` handlers swallow errors via `finally { setReviewSubmitting(false) }`. If `onReviewApprove()` or `onReviewRequestChanges()` rejects, the button simply returns to its idle state with no feedback to the user. Consider surfacing the error (e.g., via a toast or an inline error message) so the reviewer knows the action did not complete.
How can I resolve this? If you propose a fix, please make it concise.
When an issue is in the in_review status and a review handler is provided, render an 'Aguardando revisão' banner above the chat thread with two actions: - Approve (sets status=done) - Request changes (sets status=in_progress with a rejection reason) Also: replies submitted via the composer automatically reopen the issue when it is in a closed status (done/cancelled) and no explicit reopen preference was passed.
ramonmatias19
force-pushed
the
feature/issue-review-banner
branch
from
5ed3bf9 to
b0d3f0b
Compare
1 participant
Summary
When an issue is in
in_reviewstatus and a review handler is provided, render an Aguardando revisão banner above the chat thread with two actions:status=done)status=in_progresswith a rejection reason)Also: replies submitted via the composer automatically reopen the issue when it is in a closed status (
done/cancelled) and no explicitreopenpreference was passed.Test plan
in_review— verify the banner appearsdoneand a comment is addedin_progressand a comment is addeddoneissue — verify it reopens automatically