Re-label `restore-file` and use new API #6596

134130 · 2023-05-01T09:07:26Z

Description

Closes restore-file occasionally restores the wrong commit #4679
Previously, restore-file restores file to the base of the Pull Request.
- So when the Base's HEAD is changed after PR has been opened, The restoring looks oddly
But GitHub's UI shows file's diff from the commit, commit-range and pull-request-totally (all-commits).
And we usually uses git restore for restoring a file to not "untouched status", but "previous status".
Let's restore file to not untouched status but diff's original status.

Test URLs

Full files diff: https://github.com/134130/refined-github-test/pull/1/files
Range commit diff: https://github.com/134130/refined-github-test/pull/1/files/15f7fb6..5deabc1
Single commit diff: /Test 134130/refined-github-test#1/commits/7cd9c13

Maybe the above url's cannot work. Check Range commit diff and Single commit diff with this screenshot

Screenshot

Same as before.

source/features/restore-file.tsx

fregante · 2023-05-01T15:18:23Z

Maybe I'm confused, but "Restore file" should not depend on the current URL.

The intent of the button is to completely remove the file from the PR, does this happen with this code?

Examples to test:

change A.js in a PR, change A.js on the main branch, click "Update branch", click restore
The same, but use a "Rebase merge" in the branch update

Live example:

https://github.com/refined-github/sandbox/pull/29/files

134130 · 2023-05-01T23:04:23Z

Oops if the intend of the feature is "completely remove the file from the PR", This PR is not correct.
I though the "restore" means "git restore" and We need to restore file relative on commits not relative on a full pull request.

You can remove file from the PR as you did before, on Full files diff page.

I intended restore works like git.

Example situation:

The PR has 5 commits.
first commit is full refactoring commit, and the last commit is adding some notes.
You thinks the refactoring is correct, but the adding notes is not needed.
So you want to drop adding notes commit.

Also, when we use restore button on this page, How do you expect the restore to behave?

Before: restore to just 2 lines state. since removing the file from PR
On this PR: 6 and 7 lines are added. restore to 5 lines state

Examples to test:

change A.js in a PR, change A.js on the main branch, click "Update branch", click restore
- The whole file will be restored to where the PR branch is created.
- So the change A.js on the main branch will not be reflected
The same, but use a "Rebase merge" in the branch update
- The whole file will be restored to where the PR branch is created.
- So the change A.js on the main branch will be reflected

yakov116 · 2023-05-01T23:17:24Z

I did not read your entire post, this feature restores the as it was prior to the PR was submitted.

134130 · 2023-05-02T00:13:52Z

@yakov116 Then I think we need to hide restore button on showing commit's different page since the UX is different.
We need to change the button only shows on full file changes since the file will be restored to before PR submitted.

134130 · 2023-05-02T00:19:36Z

I'm not arguing for it, just for a slightly better UX. If that was the original intent, I'm willing to fix it, and it's very easy. :)

fregante · 2023-05-02T04:27:13Z

I think we need to hide restore button on showing commit's different page

All the other buttons are still available there and refer to “the file in the PR”, not “the file in the state shown in this URL.” Even the Edit button is shown.

fregante · 2023-05-02T04:36:56Z

Maybe we can make this more clear by changing the label to “Drop from PR” or “Discard changes” like GitHub Desktop calls it

134130 · 2023-05-02T08:09:20Z

Decided on the wording, I'll rework it to drop.

fregante

The changes look good on paper. Just make sure it works and update the PR title to reflect the changes (it's no longer a bugfix I think)

source/features/restore-file.tsx

yakov116 · 2023-05-04T14:19:16Z

source/features/restore-file.tsx

@@ -127,7 +123,7 @@ function handleMenuOpening({delegateTarget: dropdown}: DelegateEvent): void {
 			role="menuitem"
 			type="button"
 		>
-			Restore file
+			Drop from PR


I am not sure i like this wording, we "restoring the file" to how it was prior to the pr.

It's not clear whether it's "Based on commit" or "Based on PR".
As I said before, the UI may only show a portion of the commit, not the entire PR.

Yeah I suggested a change because "Restore" isn't really clear what we're restoring to. My suggestions were either

Drop from PR

Discard changes

But the first one is the clearest.

134130 · 2023-05-04T15:01:51Z

@fregante
"Revert file to before PR" looks pretty good.
But It seems a little long for feature's title.
What do you think of it?

While working with drop-file-from-pr, I found "Adds a button to revert all the changes to a file in a PR" description for the feature.

fregante · 2023-05-04T15:06:42Z

Yes we used to call it "Revert file" but that was just as ambiguous. That label is also too long.

source/github-helpers/get-pr-info.ts

fregante

@134130 no need to rename features, renaming is a last resort that breaks our search. Just change the label here

fregante · 2023-05-04T15:20:03Z

readme.md

-			<p><a title="restore-file"></a> Adds a button to revert all the changes to a file in a PR
-			<p><img src="https://user-images.githubusercontent.com/1402241/130660479-083e91e6-0778-446a-9aaf-b9b3e7214281.gif">
+			<p><a title="drop-file-from-pr"></a> Adds a button to revert all the changes to a file in a PR
+			<p><img src="https://user-images.githubusercontent.com/50487467/236245182-afc97200-2634-4f82-9e9c-1f605fb04565.gif">


Thanks for the gif update!

fregante · 2023-05-04T16:15:32Z

Note again: this does not fix #4679

FloEdelmann · 2023-05-04T18:10:47Z

source/features/restore-file.tsx

-		throw new Error('Nothing to restore. Delete file instead');
+		throw new Error('Nothing to drop. Delete file instead');


That error message seems wrong now: There is something to drop (the whole new file), but nothing to restore. When does this error occur?

Suggestions for better wording:

File was created in this PR. Delete it instead.

Can't drop created file. Delete it instead.

134130 · 2023-05-04T23:59:49Z

@fregante It also fixes #4679 since we change getBaseRef function.
When the base branch has some new commits after the PR is opened, We've got the head reference of base branch not the PR 's head commit.
Now, We can get the PR's head commit correctlly

fregante · 2023-05-05T01:54:26Z

I does not. The underlying API is unchanged from what I see. It used repository.pullRequest.baseRefOid before and it uses repository.pullRequest.baseRefOid now. Note that the feature that uses getPrInfo has the same exact issue, except in that case it’s unclear whether it’s the “correct” thing to show:

pr-base-commit doesn't show the most recent common commit (should it?) #6573

134130 commented May 1, 2023

View reviewed changes

source/features/restore-file.tsx Outdated Show resolved Hide resolved

fregante marked this pull request as draft May 2, 2023 04:27

134130 force-pushed the fix/#4679 branch from d5df216 to 585ba5c Compare May 4, 2023 12:58

Just fix getBaseReference()

bd778c2

134130 force-pushed the fix/#4679 branch from 585ba5c to bd778c2 Compare May 4, 2023 12:59

134130 marked this pull request as ready for review May 4, 2023 12:59

lint

4264a8e

fregante approved these changes May 4, 2023

View reviewed changes

source/features/restore-file.tsx Outdated Show resolved Hide resolved

yakov116 reviewed May 4, 2023

View reviewed changes

Drop onetime

7e5cbd4

yakov116 reviewed May 4, 2023

View reviewed changes

source/github-helpers/get-pr-info.ts Show resolved Hide resolved

Change name from restore-file to drop-file-from-pr

491a1e4

134130 changed the title ~~Fix restore-file: restores based on user's current diff~~ Rename restore-file to drop-file-from-pr May 4, 2023

fregante requested changes May 4, 2023

View reviewed changes

fregante reviewed May 4, 2023

View reviewed changes

Drop rename feature name

8374586

134130 changed the title ~~Rename restore-file to drop-file-from-pr~~ Fix: restore-file drops file from PR correctlly May 4, 2023

Lint

b3b33b9

fregante changed the title ~~Fix: restore-file drops file from PR correctlly~~ Re-label restore-file and use new API May 4, 2023

fregante added the enhancement label May 4, 2023

Merge branch 'main' into fix/refined-github#4679

d2ebe63

fregante enabled auto-merge (squash) May 4, 2023 16:19

Bad merge

afe503b

fregante merged commit f9a7935 into refined-github:main May 4, 2023
8 checks passed

FloEdelmann requested changes May 4, 2023

View reviewed changes

fregante mentioned this pull request May 6, 2023

restore-file UX improvements #6621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-label `restore-file` and use new API #6596

Re-label `restore-file` and use new API #6596

134130 commented May 1, 2023 •

edited

fregante commented May 1, 2023

134130 commented May 1, 2023

yakov116 commented May 1, 2023

134130 commented May 2, 2023 •

edited

134130 commented May 2, 2023 •

edited

fregante commented May 2, 2023

fregante commented May 2, 2023

134130 commented May 2, 2023

fregante left a comment

yakov116 May 4, 2023

134130 May 4, 2023

fregante May 4, 2023 •

edited

134130 commented May 4, 2023

fregante commented May 4, 2023

fregante left a comment

fregante May 4, 2023

fregante commented May 4, 2023

FloEdelmann May 4, 2023

134130 commented May 4, 2023 •

edited

fregante commented May 5, 2023 •

edited

		throw new Error('Nothing to restore. Delete file instead');
		throw new Error('Nothing to drop. Delete file instead');

Re-label restore-file and use new API #6596

Re-label restore-file and use new API #6596

Conversation

134130 commented May 1, 2023 • edited

Description

Test URLs

Screenshot

fregante commented May 1, 2023

134130 commented May 1, 2023

yakov116 commented May 1, 2023

134130 commented May 2, 2023 • edited

134130 commented May 2, 2023 • edited

fregante commented May 2, 2023

fregante commented May 2, 2023

134130 commented May 2, 2023

fregante left a comment

Choose a reason for hiding this comment

yakov116 May 4, 2023

Choose a reason for hiding this comment

134130 May 4, 2023

Choose a reason for hiding this comment

fregante May 4, 2023 • edited

Choose a reason for hiding this comment

134130 commented May 4, 2023

fregante commented May 4, 2023

fregante left a comment

Choose a reason for hiding this comment

fregante May 4, 2023

Choose a reason for hiding this comment

fregante commented May 4, 2023

FloEdelmann May 4, 2023

Choose a reason for hiding this comment

134130 commented May 4, 2023 • edited

fregante commented May 5, 2023 • edited

Re-label `restore-file` and use new API #6596

Re-label `restore-file` and use new API #6596

134130 commented May 1, 2023 •

edited

134130 commented May 2, 2023 •

edited

134130 commented May 2, 2023 •

edited

fregante May 4, 2023 •

edited

134130 commented May 4, 2023 •

edited

fregante commented May 5, 2023 •

edited