Skip to content

Add Request Changes button to merge queue UI#724

Open
iamnbutler wants to merge 1 commit intomainfrom
tasks/gh-iamnbutler-tasks-issue-520--b072ab12
Open

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

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
  • Clicking opens a dialog where the user can enter feedback describing what needs to change
  • Feedback is sent to the agent session via the /api/merge-queue/:id/request-changes endpoint

Test plan

  • Verify the merge queue page loads without errors
  • Confirm "Request Changes" button appears for pending entries alongside Approve and Reject
  • Click "Request Changes", enter feedback, submit — verify entry transitions to changes_requested status
  • Verify canceling the dialog does not trigger an API call
  • Verify submitting with empty feedback is disabled

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 for
pending entries. Clicking opens a dialog to enter feedback, which is
sent back to the agent session via the /request-changes endpoint.

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.

The page.tsx Dialog implementation is solid — proper error handling with toast notifications, loading/submitting state, feedback cleared on success, and the changes_requested_feedback display in the row is a nice touch. Core functionality looks correct end-to-end.

Two suggestions worth considering before/after merge:

  1. Dead code in columns.tsx — the columns export (TanStack Table definitions) isn't consumed anywhere in the current app. page.tsx imports only utility functions from columns.tsx. The handleRequestChanges using window.prompt() added to this file will never run. If the table view is meant to stay, it should use the same dialog approach and add error handling to match page.tsx.

  2. Feedback state not reset on Cancel — minor UX inconsistency: the success path clears feedback but the Cancel button doesn't. See inline comment for the one-liner fix.

The reasoning and feedback API fields both receiving the same user input is a reasonable simplification for human-originated requests — reasoning ends up in the event log for auditing while feedback is what the agent sees, and for a human entering one text field, duplicating the value is fine.


References:

Reviewed by PR / Review

}

async function handleRequestChanges() {
const feedback = window.prompt("Describe the changes needed:");
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]

Priority: Code Quality

The columns export (TanStack Table column definitions) is not imported by any active consumer — page.tsx only imports utility functions from this file (statusBadge, lifecyclePhases, etc.), not the columns array. This means handleRequestChanges using window.prompt() is dead code and will never run in the current UI.

If the table view is intended to be revived, this implementation has two problems compared to the page.tsx dialog approach: it uses a blocking native browser prompt (can be suppressed by browsers, unstyled, inconsistent UX) and it has no error handling — if requestChanges() throws, the error propagates silently.

Consider either wiring up the columns export to a table view, or removing the handleRequestChanges from columns.tsx entirely since the dialog in page.tsx handles this correctly.

<Button
variant="outline"
size="sm"
onClick={() => setRequestChangesOpen(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]

Priority: Code Quality

The Cancel button closes the dialog but doesn't reset the feedback state:

onClick={() => setRequestChangesOpen(false)}

If a user types some feedback, cancels, then reopens the dialog for the same entry, the previous draft text is still present. The success path correctly clears feedback (setFeedback("")), so the Cancel path is inconsistent with that expectation.

Consider resetting on close: onClick={() => { setRequestChangesOpen(false); setFeedback(""); }}. Similarly, you could add a cleanup to onOpenChange: onOpenChange={(open) => { if (!open) setFeedback(""); setRequestChangesOpen(open); }}.

@iamnbutler
Copy link
Copy Markdown
Owner Author

Orchestrator Evaluation: Rejected

The PR addresses the issue by wiring up the requestChanges API function into both the columns.tsx (table view) and page.tsx (list view) UI components. The implementation in page.tsx is solid — proper Dialog with textarea, loading states, error handling with toast, and feedback clearing on success. The display of changes_requested_feedback in the row is a nice touch. However, there are two issues that prevent approval:

  1. CI is FAILING — the metadata explicitly states CI status is failing. This is a hard blocker.

  2. columns.tsx uses window.prompt for feedback — In the table/columns view, the handleRequestChanges function uses window.prompt() which is a poor UX pattern inconsistent with the Dialog approach used in page.tsx. More importantly, the columns.tsx implementation has no error handling (no try/catch around the requestChanges call), unlike page.tsx which properly catches errors and shows toast notifications. If the API call fails in the columns view, it will throw an unhandled promise rejection.

  3. Minor: duplicate argumentrequestChanges(entry.id, feedback.trim(), feedback.trim()) passes the same feedback string as both the second and third arguments. This appears in both files. Without seeing the API function signature, this looks suspicious — is the third argument supposed to be something different (like a summary vs detailed feedback)?


Feedback for agent:

  1. CI is failing — this must be resolved before merge.

  2. columns.tsx: No error handlinghandleRequestChanges in columns.tsx (line 283-288) has no try/catch around the requestChanges() call, unlike the page.tsx implementation which properly catches errors and shows a toast. An API failure will result in an unhandled promise rejection.

  3. columns.tsx: window.prompt is inconsistent — The table view uses window.prompt() while the list view uses a proper Dialog. Consider using a consistent approach, or at minimum add error handling to the columns.tsx version.

  4. Duplicate argument — Both call sites pass feedback.trim() as both the 2nd and 3rd argument to requestChanges(). Is this intentional? Please verify the API signature — if the 3rd param is meant to be something different (e.g., a summary), this is a bug.

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