Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #5179: [frontend]: Push to Github button should only push branch, but not creating a PR #5181

Merged
merged 15 commits into from
Nov 25, 2024

Conversation

openhands-agent
Copy link
Contributor

@openhands-agent openhands-agent commented Nov 21, 2024

This pull request fixes #5179.

The PR has successfully resolved the original issue by implementing the requested split functionality for GitHub interactions. Specifically:

  1. The original single "Push to GitHub" button that automatically created PRs has been separated into two distinct buttons:

    • A "Push to GitHub" button that only pushes changes to the remote branch
    • A "Push & Create PR" button that handles both pushing and PR creation
  2. The implementation includes proper test coverage to verify:

    • The presence of both buttons when GitHub credentials are available
    • Correct button labeling
    • Proper message handling when each button is clicked
  3. The changes maintain backward compatibility while adding the requested granular control over GitHub operations.

This solution directly addresses the original problem by preventing unintended PR creation while still maintaining the ability to create PRs when desired. The clear separation of concerns through distinct buttons provides a better user experience aligned with the requested solution.

A reviewer can verify that:

  • Both buttons are properly implemented and visible
  • Each button triggers the correct corresponding action
  • The test suite covers the new functionality
  • The implementation doesn't break existing features

Automatic fix generated by OpenHands 🙌


It looks something like this:

image

Before click push to PR:

image

Once Push to PR is clicked
image


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:4df8b6f-nikolaik   --name openhands-app-4df8b6f   docker.all-hands.dev/all-hands-ai/openhands:4df8b6f

Copy link
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

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

@openhands-agent you should do npm run lint:fix to fix frontend linting.

@xingyaoww xingyaoww added the fix-me Attempt to fix this issue with OpenHands label Nov 21, 2024
Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

…ld only push branch, but not creating a PR
@openhands-agent
Copy link
Contributor Author

New OpenHands update

Copy link
Contributor

The workflow to fix this issue encountered an error. Please check the workflow logs for more information.

Copy link
Contributor

The workflow to fix this issue encountered an error. Please check the workflow logs for more information.

Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

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

Could we get a screenshot of the resulting design? I'm a little bit worried about the design becoming noisy.

@xingyaoww
Copy link
Collaborator

@neubig edited the PR description to include a screenshot

@neubig
Copy link
Contributor

neubig commented Nov 22, 2024

Thanks! @rbren , wdyt about this design?

For me personally I think it might be nice if we could put the buttons side-by-side to save screen space?

@xingyaoww
Copy link
Collaborator

@openhands-agent Can you put these two button side to side?

Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

…ld only push branch, but not creating a PR
@openhands-agent
Copy link
Contributor Author

New OpenHands update

Copy link
Contributor

The workflow to fix this issue encountered an error. Please check the workflow logs for more information.

@xingyaoww
Copy link
Collaborator

@openhands-agent please display the "Push to Github" and "Push & Create PR" button side-by-side
image

Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

@xingyaoww xingyaoww added fix-me Attempt to fix this issue with OpenHands and removed fix-me Attempt to fix this issue with OpenHands labels Nov 23, 2024
Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

…ld only push branch, but not creating a PR
@openhands-agent
Copy link
Contributor Author

New OpenHands update

Copy link
Contributor

The workflow to fix this issue encountered an error. Please check the workflow logs for more information.

Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

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

LGTM now!

suggestion={{
label: "Push to GitHub",
value:
"Please push the changes to a remote branch on GitHub, but do NOT create a pull request.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we update the microagent instructions to send the user a compare link?

e.g.

If the user asks you to push your changes, but not create a PR, send them a message with a link to compare the files on the remote branch against the main branch.

];
renderChatInterface(messages);

const pushButton = screen.getByRole("button", { name: "Push to GitHub" });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's name this Push to Branch

Copy link
Collaborator

Choose a reason for hiding this comment

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

done

renderChatInterface(messages);

const pushButton = screen.getByRole("button", { name: "Push to GitHub" });
const prButton = screen.getByRole("button", { name: "Push & Create PR" });
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think once the Push & Create PR button has been clicked once, we should only show one button after that. The agent (and user) will get v confused if you push and create PR, and then click "push to branch", since the PR is already opened.

Once you click "Push & Create PR" we should just have one button that says "Push changes to PR"

Copy link
Collaborator

Choose a reason for hiding this comment

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

done the most recent commit

@xingyaoww
Copy link
Collaborator

@openhands-agent Can you review the comments above and work on it?

Copy link
Contributor

OpenHands started fixing the pr! You can monitor the progress here.

@enyst
Copy link
Collaborator

enyst commented Nov 25, 2024

@xingyaoww Hah, sorry, I have the assumption that it will read all comments too, but it doesn't automatically receive them: #5236

…ld only push branch, but not creating a PR
@openhands-agent
Copy link
Contributor Author

New OpenHands update

Copy link
Contributor

The workflow to fix this issue encountered an error. Please check the workflow logs for more information.

openhands-agent and others added 4 commits November 25, 2024 15:20
… creation

* Rename 'Push to GitHub' button to 'Push to Branch'
* Add state to track PR creation
* Show only 'Push changes to PR' button after PR is created
* Update tests to reflect new behavior
@xingyaoww
Copy link
Collaborator

Before click push to PR:

image

Once Push to PR is clicked
image

@xingyaoww xingyaoww marked this pull request as ready for review November 25, 2024 15:40
@xingyaoww xingyaoww enabled auto-merge (squash) November 25, 2024 15:51
@xingyaoww xingyaoww merged commit d267c06 into main Nov 25, 2024
17 checks passed
@xingyaoww xingyaoww deleted the openhands-fix-issue-5179 branch November 25, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix-me Attempt to fix this issue with OpenHands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[frontend]: Push to Github button should only push branch, but not creating a PR
5 participants