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.

Sign up for GitHub

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

Jump to bottom

Use correct file after merges in in restore-file #6670

Merged
merged 5 commits into from May 20, 2023
Merged

Use correct file after merges in in restore-file #6670

merged 5 commits into from May 20, 2023

Conversation

fregante
Copy link
Member

@fregante fregante commented May 20, 2023 •
edited

This is the generated call:

mutation {
  createCommitOnBranch(
    input: {
      branch: {
        repositoryNameWithOwner: "refined-github/sandbox"
        branchName: "4679-2"
      }
      expectedHeadOid: "6ad16fb97fd1a0dee13d5177cb0bb95448bfb2a5"
      fileChanges: {
        additions: [{ path: "4679", contents: "b3JpZ2luYWwgZmlsZQo=" }]
      }
      message: { headline: "Discard changes to 4679" }
    }
  ) {
    commit {
      oid
    }
  }
}

@fregante fregante added the bug label May 20, 2023
fregante
fregante commented May 20, 2023
View reviewed changes
Copy link
Member Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

I don't know exactly how to reproduce, it seems that the old "update branch" method no longer broke the feature. Maybe:

  1. Edit file in PR
  2. Edit in MAIN
  3. Change PR base to some random branch
  4. Change PR base back to MAIN (I think this basically recalculates the base commit and breaks pr-base-commit while we're at it…)
  5. Click "Discard changes"

Before

This video starts with an already-wrong restore situation. +2 was only added to main recently

Screen.Recording.9.mov

After

This also starts in that broken situation, but fixes it

Screen.Recording.10.mov

source/features/restore-file.tsx
if (editFile.closest('.file-header')!.querySelector('[aria-label="File added"]')) {
// The file is new. "Discarding changes" means deleting it, which is already possible.
// Depends on `highlight-deleted-and-added-files-in-diffs`.
return;
Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped this exclusion. Our pattern is still faster than GitHub’s, which loads a whole new page and prompts for the commit title.

Tested on https://github.com/refined-github/sandbox/pull/16/files

Screen.Recording.2.mov

source/features/restore-file.tsx
editFile.after(
<button
className="pl-5 dropdown-item btn-link rgh-restore-file"
style={{whiteSpace: 'pre-wrap'}}
Copy link
Member Author

Choose a reason for hiding this comment

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

🤷‍♂️

source/features/restore-file.tsx
Comment on lines +120 to -137
observe('.js-file-header-dropdown a[aria-label^="Change this"]', add, {signal});

// `capture: true` required to be fired before GitHub's handlers
delegate('.file-header .js-file-header-dropdown', 'toggle', handleMenuOpening, {capture: true, signal});
Copy link
Member Author

Choose a reason for hiding this comment

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

No more toggle listeners please:

source/features/restore-file.tsx
Copy link
Member Author

Choose a reason for hiding this comment

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

Standard behavior still working, tested on https://github.com/refined-github/sandbox/pull/16/files

Screen.Recording.3.mov

@fregante fregante marked this pull request as ready for review May 20, 2023 18:00
@fregante
Copy link
Member Author

fregante commented May 20, 2023 •
edited

YOL9T 🐈‍⬛

@fregante fregante merged commit 8dc69ec into main May 20, 2023
11 checks passed
@fregante fregante deleted the restore-merged-file branch May 20, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

restore-file occasionally restores the wrong commit
1 participant
@fregante