Fix infinite loops in compare page

[willdurand]

Fixes #638
Fixes #628

---

While the code looked nice and the test suite was covering all the branches, it appears that most of the code was not needed because loading a file from the URL is done by fetchDiff, which takes an optional path. Given that there is no URL update triggered by our code, the URL stayed the same and everything just worked as intended. This was nice but complicated and not needed, so I removed it.

The fix for the infinite loop is mostly in src/reducers/versions.tsx. Because we load a file, we should abort this action too in case of an error. This allows the loadData() method to check the status of the version prop.

I added some commits to fix #628 too. I believe it works (i.e. I cannot trigger an infinite loop) but the UI sometimes lags. I covered the following scenarios:

• loading an URL with a path in the query string, http://localhost:3000/en-US/compare/502955/versions/1541794...1541798/?path=META-INF%2Fmanifest.mf
• loading an URL with a path in the query string and a hash (for the line), http://localhost:3000/en-US/compare/502955/versions/1541794...1541798/?path=META-INF%2Fmanifest.mf#I55
• loading a default compare page, http://localhost:3000/en-US/compare/502955/versions/1541794...1541798/
• loading a compare page and hitting j twice to end up in a file with some content, http://localhost:3000/en-US/compare/502955/versions/1541794...1541798/?path=META-INF%2Fcose.manifest

I think protecting the thunk with a loading flag would help. I've done that in the last commit.

[codecov-io]

Codecov Report

Merging #639 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
+ Coverage   98.42%   98.43%   +0.01%     
==========================================
  Files          45       45              
  Lines        1267     1279      +12     
  Branches      292      292              
==========================================
+ Hits         1247     1259      +12     
  Misses         19       19              
  Partials        1        1

Impacted Files	Coverage Δ
src/reducers/versions.tsx	100% <100%> (ø)	⬆️
src/pages/Compare/index.tsx	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ec042c...fdcc3ed. Read the comment docs.

[willdurand]

Looks like codecov did not update the coverage report according to the last pushed code...

[kumar303]

Just so I don't forget what I said about this on IRC 😀 could you look into #628 first? Your patch here seems to be making it worse. I do like how you simplified the initial path loading, though. We should definitely do that.

[willdurand]

@kumar303 could you please test this patch? I am not sure if I am the only one seeing UI lags (cf. PR description). It looks like it ends up in the correct state though, but it takes time...

[willdurand]

@kumar303 could you please test this patch? I am not sure if I am the only one seeing UI lags (cf. PR description). It looks like it ends up in the correct state though, but it takes time...

I've fixed this a bit by protecting the fetchDiff thunk with a loading flag, but the file tree still flickers a bit when we hit k too rapidly. I believe this is a separate issue, though.

[kumar303]

...the file tree still flickers a bit when we hit k too rapidly. I believe this is a separate issue, though.

Can you file an issue for it? It's because the thunk is loading in the background for each file / diff you traverse. The flicker happens when one of the older diffs finally loads but a newer one loads right after it. I think this means we need debouncing. It may not be trivial to add debouncing.

[willdurand]

Can you file an issue for it?

Yes. Edit: #644

It's because the thunk is loading in the background for each file / diff you traverse. The flicker happens when one of the older diffs finally loads but a newer one loads right after it. I think this means we need debouncing. It may not be trivial to add debouncing.

Maybe we could debounce the keyboard navigation component instead (the key listener maybe?).

[kumar303]

This is way simpler, I like it!

It works great, I've just requested some fixes to the tests.

[willdurand]

Thanks @kumar303, I believe I addressed all the change requests. There is one for which I could not do anything.

[kumar303]

This is great