1. In Firefox, clicking any "Show comment" in Move loading logic into each feature #1694 freezes the browser for 4 seconds in my case, but not in Chrome. Can you look into it?

2. Also review comments don't work in Chrome and Firefox, here https://github.com/sindresorhus/refined-github/pull/1896/files

3. The suggested link for large diffs only has one large diff on the page: https://github.com/parcel-bundler/parcel/pull/2967/files

3. The suggested link for large diffs only has one large diff on the page: parcel-bundler/parcel/pull/2967/files

There are multiple of them on that page. It is the same as loading diffs for deleted files: https://github.com/sindresorhus/refined-github/pull/1524/files.

1. In Firefox, clicking any "Show comment" in Move loading logic into each feature #1694 freezes the browser for 4 seconds in my case, but not in Chrome. Can you look into it?

Happens to me on Chrome too. Not a bug, just main thread jank for clicking on 90 elements all at once. (Any solution?)

2. Also review comments don't work in Chrome and Firefox, here sindresorhus/refined-github/pull/1896/files

Missed that, added it in.

Happens to me on Chrome too. Not a bug, just main thread jank for clicking on 90 elements all at once. (Any solution?)

It's worth investigating if it's the click or the selection being slow

It's the clicks, events being triggered recursively. But the functions early return anyway.

Should we space them out / delay them a bit? We can't cause multi-second freezes.

In this case I'd probably click no more than 3 per second, also because it's gonna take longer than that to open + see them anyway, but only the "Load diff". You can probably implement this by using await delay(300) in the loop: https://www.npmjs.com/package/delay

Additionally, perhaps, it could be limited to 10 files at a time, but this code might be a bit ugly and maybe not worth it.

Also can you rename this to toggle-everything-with-alt?

Can you add the same mechanism here? https://github.com/sindresorhus/refined-github/pull/2073/files#diff-bfeb5de166cf585c43d9150166877a54R89

Except spaced every 3 and using requestAnimationFrame instead

Can you add the same mechanism here? sindresorhus/refined-github/pull/2073/files#diff-bfeb5de166cf585c43d9150166877a54R89

The handlers are delegated over to .repository-content, so this works even for ajaxed file diffs in a PR even without using that util.

Except spaced every 3 and using requestAnimationFrame instead

I don't seem to follow you on this.

If you are talking about #2087 (comment), then that's not possible, as the possible solution in #2073 mentioned would not work here. Spacing out callback to let the user scroll will cause the last scroll position to be invalidated and when the callback is eventually fired in rAF, the page scrolls to random positions for each callback.

Having a jank is pretty reasonable IMHO (instead of handling scroll position changes for random file load jumps).

There might have been some miscommunication. My comment only was referring to the await promise setTimeout mechanism (652bb9e#diff-bfeb5de166cf585c43d9150166877a54R90) but the link I had posted pointed to the wrong line (probably after a commit changed its line)

I'm sorry, I think this feature can no longer be made of just 2 functions. In its first iteration last year, all toggles behaved similarly, now it seems that each one needs to behave differently because we keep fixing one side and breaking the other.

Example in Chrome (you can see the menus flashing):

In Firefox it's way worse as it takes longer and the scroll goes nuts.

How about this: let's focus on one feature and get that one in first, for example #825

Ensure that it works in both browsers and without long stalls, we'll merge it.

You can leave the name as toggle-everything-with-alt at the moment. Probably you can't use any scroll anchoring because the loads are asynchronous.

I have good news for this:

• Review comments per-file ("Show/Hide comments"): sindresorhus/refined-github/pull/1896/files.

Toggle can be simulated via:

document.dispatchEvent(new KeyboardEvent('keydown', {code: 'KeyI', key: 'i'}))

The i key will toggle all File comments, it's a native GitHub shortcut. Hopefully this can be integrated easily (in the next PR). Goes to show how each part has its own way to do things.

Ping on this if you have time

Ping on this if you have time

Sorry I couldn't reply on this, I've been away from keyboard lately because of a lot of distractions. I'll try to get back to handling things as fast as I can.

I thought of closing this PR and opening a new one as it was getting a bit complex with way too many reviews, but I think I found solutions to the above issues, so left it as is.

Example in Chrome (you can see the menus flashing):
...
In Firefox it's way worse as it takes longer and the scroll goes nuts.

Found the source of that bug, I was targeting the summary element deep within the hidden comment, which is also that dropdown.

I have good news for this:
...
Toggle can be simulated via:
...
The i key will toggle all File comments, it's a native GitHub shortcut. Hopefully this can be integrated easily (in the next PR). Goes to show how each part has its own way to do things.

That still has the issues mentioned above in #2087 (comment), the focus keeps shifting to the last file in the list, adding/removing is still the better way to go.

Tested in Firefox.

• Hidden comments: Move loading logic into each feature #1694

This still freezes the browser for 1s+ to toggle all of those... but that's passable.

• Review comments in PR files ("Show/Hide conversation"): sindresorhus/refined-github/pull/1896/files

Seems to work correctly.

• Review comments per-file ("Show/Hide comments"): sindresorhus/refined-github/pull/1896/files

Does not work. The menu stays open as if the click was not registered at all. Previously reported in #2087 (comment)

• Loading large diffs at once: parcel-bundler/parcel/pull/2967/files

Seems to work correctly.

As said before, I was no a fan of all the if in those two functions, they handled too much.

I rewrote the feature trying to split it where it made sense and it's a bit shorter and readable.

However I dropped the part that wasn't working (as per my previous comment) and given how complex it was, I'd rather see it replaced by something like

anchorScroll(() => {
    document.dispatchEvent(new KeyboardEvent('keydown', {code: 'KeyI', key: 'i'}))
}, clickedElement.closest('.js-file'));

But that can come in a secondary PR. I'll test this further in both browser and I'll merge it soon.