Refined-github: Save collapsed files in a pull request diff

[tmosmant]

That way we could easily resume a review.

[sindresorhus]

What if those files are updated in new commits? They would be collapsed, but would probably need another review.

[tmosmant]

True, in this case they should uncollapse.

[DrewML]

@tmosmant I could be wrong, but I don't think the UI gives us any hints that would allow us to determine if a file has changed. Happy to be proven wrong :)

[tmosmant]

You have the View button on each diff which contains the whole commit hash. I think we can use it for that purpose.

[DrewML]

Nice! Good call. I guess we'd have to start persisting more info in chrome.storage for this feature, but that doesn't seem like the worst thing in the world.

@sindresorhus @paulmolluzzo thoughts?

[sindresorhus]

I'm -0. Wouldn't use it, but don't feel strongly about not having it either.

[paulmolluzzo]

If I come back to a PR and there are changes that are hidden because the file is collapsed that'd be annoying (or worse).

You have the View button on each diff which contains the whole commit hash. I think we can use it for that purpose.

I guess we'd have to start persisting more info in chrome.storage for this feature, but that doesn't seem like the worst thing in the world.

What would we persist and test for? That the current commit is the same as the hash on the button? You'd have to reset the state for all files if someone rebases or amends and forces an update, so there's that to consider too (though the test I mentioned would do that, I think).

It should also err on the side of staying open because it's way worse to overlook a change before merging than clicking "hide" over again.

And it's probably trivial, but what about cleaning out storage? Is something going to be saved every time you visit a PR page?

[tmosmant]

I've already worked on this problem and the solution I came up with was to save a generated ID like {PR-ID}{COMMIT-HASH}{FILE-PATH} in the localStorage every time the user collapsed a diff, and remove it when uncollapsing.

[tmosmant]

Missclick!

[DrewML]

And it's probably trivial, but what about cleaning out storage? Is something going to be saved every time you visit a PR page?

That was my only concern with this. I had a similar concern when doing #251, but didn't really address it because it's data that is likely still valuable days/weeks/month down the line.

@tmosmant mentioned removing the data on uncollapse (I assume that means both manual uncollapse and automated on page load), which would likely take care of a good chunk of data.

According to this page, our extension (using chrome.storage.local) has about 5MB total storage space before we start hitting errors.

[tmosmant]

My suggestion is that we can remove data on manual uncollapse and also have an expiration date. At page load, data older than x days can be removed - does a week sounds good?

[hkdobrev]

I don't see much benefit in this as it's more about the commits than the files. GitHub has added functionality to the conversation timeline showing new things which you haven't seen and other good stuff there. Collapsing files and trying to uncollapse is fragile, but also doesn't show you the whole picture. You could have added a new file in a PR in one commit and then remove that file in a subsequent commit. You wouldn't see this in the Files changed tab when looking at all changes of the PR. So the benefit is not that much for me as I'd be using the timeline to see what to review in a big long-standing open PR. The potential harm in collapsing something which we shouldn't is too big for me and the cost of the effort of building it properly and maintaining it is too high to justify it.

That said, 👎 from me.

[fregante]

Closing because of inactivity, lack of PR, and generally 👎