GitHub remove diff signs: not all files get their signs removed

[duianto]

Hello, I just found the "GitHub remove diff signs" userscript, thanks for making it.

But it seems to have a bug, the + and - signs doesn't get removed from all files in the "Files changed" tab of some pull requests.

Here's an example with 6 files:
https:/Mottie/tablesorter/pull/195/files
the signs have been removed from the first 5 files, but they are still shown in the last file: js/jquery.tablesorter.js.

And here's another example with 17 files:
https:/Mottie/tablesorter/pull/1293/files
the signs have been removed from the first 3 files, but they are still there from the 4th file: js/widgets/widget-filter-formatter-html5.js down.

Here's an example with 4 files. The first two files are hidden until one clicks on them:
https:/Mottie/Keyboard/pull/363/files
the first one says:

"Load diff
Large diffs are not rendered by default."

and the second file says:

"Load diff
Some generated files are not rendered by default. Learn more."

The signs on the last 2 files are shown. When one clicks on the first 2 files to show their diffs, then the signs are still visible on them as well.

The script seems to work fine on the pull requests I've seen with only 1 or 2 files.

System info

Windows 10 1607 (OS Build 14393.953)
Google Chrome 56.0.2924.87 (Official Build) (32-bit)
Tampermonkey v4.2.7
GitHub Remove Diff Signs 1.0.2

[Mottie]

Hi @duianto!

Thanks for letting me know! I've been trying to fix this issue... give me a little more time to get it working properly in all cases.

[Mottie]

Ok, I think I got it all fixed...

I've noticed that sometimes, when navigating from another page or tab to the "Files changed" tab, the dynamically loaded content doesn't always get processed. I added a 500ms delay which may, or may not process the new content; and attempts to get it to alway work have come up short.

Reloading the page should always work.

[jerone]

And using pjax-event doesn't solve it?

[Mottie]

@jerone I was not originally using pjax: events because they weren't firing in the situations outlined below; but using only mutation observers turned out to be not so great for performance. In this last update, I switched to using a mixed method which still isn't ideal.

You probably already know most of the information below, but I'm sharing all of my findings so that other developers may benefit from my research in this topic. I'm sure there are better solutions, but I was going cross-eyed from looking at GitHub's minified javascript.

pjax containers

The pjax:end event only fires when the #js-repo-pjax-container or #js-pjax-container content is updated. These overall content blocks are updated when changing pages on GitHub.

Progressive containers

Files changed

There are two progressive containers in the "Files changed" tab. The first container usually has content (less than 4 files), while the second loads in more content dynamically.

To detect changes here, I had to set up MutationObservers on the .js-diff-progressive-container and .js-diff-load-container elements. I filtered out all changes that did not involve either the .js-diff-load-container or .blob-wrapper elements. A 500 ms delay was added here as the observer would appear to fire before the content had completed rendering. I tried using 300 ms but it wouldn't catch the second container updates after switching from either the "Conversation" or "Commits" tab to the "Files changed" tab. Refreshing the browser on the "Files changed" tab almost always works if it didn't catch everything while switching tabs.

Discussion blocks

When issue discussions get really long, a "View more" (.timeline-progressive-disclosure-button) button is added in the timeline (example) to allow progressive loading of comments. This button only loads 200 additional comments, so listening for a button click isn't ideal here.

In this situation, a mutation observer targets the .js-discussion wrapper element for updates. In my code, I haven't filtered out all changes within this element, but it would be a good idea since changing reactions causes the observer to fire off.

Previews

Comment previews fire off a "preview:setup" event on the document when a "Preview" tab is hovered or clicked. Sadly, a document "preview:render" event only fires when the keyboard shortcut (ctrl shift p) key is used.

I ended up listening for the "preview:setup" event after a 500ms delay... again, not an ideal solution 😞.

---

I think I'll copy the above content over into a wiki page.

[duianto]

I updated the script in Tampermonkey and now it says:
GitHub Remove Diff Signs 1.1.0 (last updated 2h)

Ok, I think I got it all fixed...

Unfortunately there doesn't seem to have been any change in the first two example links above, the same number of files have their signs removed and the rest are still showing signs.

The third example also look the same when the page first loads, the last two visible files show signs. But when one clicks on "Load diff ..." on one of the two hidden files to show the code, then the revealed code shows the signs for half a second (as expected with the added 500ms delay), then the signs for all three visible files get removed. And when the "Load diff ..." is clicked to show the code for the last hidden file, then it also shows the signs for half a second before they are removed.

Reloading the page should always work.

Nothing changes in any of the 3 examples when the pages are reloaded.

I took a screenshot of the Network tab in Chromes Developer Tools when the first example link loads:
https:/Mottie/tablesorter/pull/195/files

The content.min.css file, says (from disk cache), i tried holding down any combination of Ctrl and Shift to not load from cache but that file always loads from cache. When i held down Alt then all the files in the Initiator column were loaded from cache.

[Mottie]

Odd, I've been testing the userscript on all 3 of those pages...

[jerone]

I can see it working 3 out of 4 times with above example page loading.

[Mottie]

It's working every time for me. Internally there is a loop that removes the +/- signs at intervals to allow continued user interaction with the page.

[jerone]

This page is consistently not working: https:/Mottie/Keyboard/pull/363/files
No errors in console.

Edit: only at load time, after opening one of the hidden diffs it works.

[Mottie]

Which blocks?

[jerone]

@Mottie commented on 25 mrt. 2017 16:31 CET:

Which blocks?

All blocks, but only at load time. After force opening one of the hidden diffs, all signs are removed.

[duianto]

I disabled all but one chrome extension tampermonkey, closed all but this tab, restarted chrome, turned off all other applications, including the once that were running in the system tray, i even restarted the computer and tested the examples between each step. The first two examples always work as described in the initial report. But the third example sometimes open with the 2 visible files having their signs removed. But every time i refresh the third examples tab, then the signs are shown again with or without Ctrl or Shift held down to not load from cache (maybe it shouldn't matter).

[jerone]

I'm on GreaseMonkey in Firefox. Also tested with and without cache. Calling processDiff() function manually does remove the signs.

[Mottie]

Derp... ok I think I see the problem LOL. Odd that it was working fine in Chrome, but not in Firefox.

[duianto]

I added these logs before and after the if query selector line:

        console.log("pre .highlight");
		if (document.querySelector(".highlight")) {
            console.log("in .highlight");

and when the page:
https:/Mottie/Keyboard/pull/363/files

loads then only the "pre .highlight" message appears in the developer tools console, but when i click on "Load diff" to show the code for one of the two files, then both the pre and in messages are shown in the console.

[Mottie]

Try the update I just pushed.

[duianto]

Yep now it seems to be working 😄 👍

[jerone]

All examples on this page are working for me too now 🎉

[Mottie]

I just added a How to wiki page with my current take on how I deal with GitHub and progressively loaded content. Let me know what you guys think.