Skip to content

Add Request Changes button to merge queue UI#722

Closed
iamnbutler wants to merge 1 commit intomainfrom
tasks/gh-iamnbutler-tasks-issue-520--6b5e53be
Closed

Add Request Changes button to merge queue UI#722
iamnbutler wants to merge 1 commit intomainfrom
tasks/gh-iamnbutler-tasks-issue-520--6b5e53be

Conversation

@iamnbutler
Copy link
Copy Markdown
Owner

Summary

  • Wires up the existing requestChanges() API function (api.ts:78-88) in the merge queue UI
  • Adds a "Request Changes" button between Approve and Reject for pending entries in both the list view (page.tsx) and table view (columns.tsx)
  • Opens a dialog to collect feedback, which is sent to the backend's /api/merge-queue/:id/request-changes endpoint
  • The task is then re-dispatched with the reviewer's feedback

Test plan

  • Navigate to the merge queue page with pending entries
  • Verify "Request Changes" button appears between Approve and Reject
  • Click "Request Changes" and verify the dialog opens
  • Verify Submit is disabled when feedback is empty
  • Enter feedback and submit — verify the entry transitions to changes_requested status
  • Verify the feedback text appears as a subtitle on the entry row

Closes #520

🤖 Generated with Claude Code

Wire up the existing requestChanges() API function in the merge queue
UI. Adds a "Request Changes" button between Approve and Reject that
opens a dialog for entering feedback. The feedback is sent to the
backend which re-dispatches the task with the requested changes.

Closes #520

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR adds a Request Changes button with a feedback dialog to the merge queue UI — the core feature works correctly. One important issue and two suggestions.

Overview: The changes touch two views of the merge queue (table view in columns.tsx, list view in page.tsx). The table view correctly extracts a MergeActions component to hold dialog state; the list view duplicates the same logic inline instead of reusing it.


[IMPORTANT] MergeActions.handleApprove / handleReject silently swallow errors — no toast.error on failure, unlike the equivalent handlers in page.tsx. Table-view users won't know if approve or reject failed. See inline comment on columns.tsx:144.

[SUGGESTION] requestChanges(id, rcFeedback.trim(), rcFeedback.trim()) passes the same value for both reasoning and feedback. These are distinct fields in the API — reasoning ends up in the event log, feedback is stored on the entry and sent to the agent. Either collapse them in the API or surface both in the UI. See columns.tsx:153.

[SUGGESTION] The Request Changes dialog state + handler is duplicated between MergeActions (columns.tsx) and MergeQueueRow (page.tsx). Exporting MergeActions and reusing it in the list view would eliminate the duplication. See page.tsx:94.

Reviewed by PR / Review

async function handleRequestChanges() {
if (!rcFeedback.trim()) return;
setRcSubmitting(true);
try {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[IMPORTANT] — Correctness

handleApprove and handleReject here have no try/catch. If the API call fails, the error is silently swallowed and the user sees no feedback. The equivalent handlers in page.tsx's MergeQueueRow both have toast.error(...) on failure — this component should too.

async function handleApprove() {
  try {
    await approveMerge(entryId);
    onRefresh();
  } catch {
    toast.error("Failed to approve merge entry");
  }
}

async function handleReject() {
  try {
    await rejectMerge(entryId);
    onRefresh();
  } catch {
    toast.error("Failed to reject merge entry");
  }
}

toast.error("Failed to request changes");
} finally {
setRcSubmitting(false);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION] — Code Quality

reasoning and feedback are passed the same value. The API accepts two semantically distinct fields — reasoning ("why changes are needed", used in the event log for observability) and feedback ("specific actionable instructions for the agent", stored on the entry and re-dispatched). Passing the same string to both is harmless today, but it means the event log contains a duplicate. Worth either:

  1. Collapsing the API to a single field if the distinction has no practical value, or
  2. Surfacing a second field in the UI if the distinction matters.

Same pattern in page.tsx:114.

@@ -84,6 +94,25 @@ function MergeQueueRow({
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION] — Code Quality

This dialog logic (rcOpen, rcFeedback, rcSubmitting, handleRequestChanges, and the full Dialog JSX below) is identical to what was just extracted into MergeActions in columns.tsx. Rather than duplicating it inline here, MergeActions could be exported and reused:

// columns.tsx
export function MergeActions({ entryId, onRefresh }: { entryId: string; onRefresh: () => void }) {  }
// page.tsx (inside MergeQueueRow actions section)
(MergeActions entryId={entry.id} onRefresh={onRefresh} /)

Two copies means the next change to this dialog has to be made twice.

@iamnbutler
Copy link
Copy Markdown
Owner Author

Closing duplicate - newer PR #724 exists

@iamnbutler iamnbutler closed this Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Web UI: requestChanges API endpoint wired but not exposed in merge queue UI

1 participant