RepoSense icon indicating copy to clipboard operation
RepoSense copied to clipboard

[#1748] Lazy loading for code segments in authorship view

Open gok99 opened this issue 2 years ago • 13 comments

Partially fixes #1748

Note: This PR also supersedes the fix in #1773 (in case this one somehow gets merged earlier than that)

Proposed commit message

Code segments are loaded all at once in the authorship view which could
take a long time or possibly break the report for especially large diffs.

Let's implement lazy loading by only rendering code segments that are
visible to the user, to speed up report loading. Let's also replace the
overall character limit with a single file character limit, since a
large number of small code segments slowing down report loading is no
longer a concern.

gok99 avatar Apr 03 '22 20:04 gok99

The relevant PR has been merged but there still seems to be merge conflicts?

gerhean avatar Apr 15 '22 16:04 gerhean

Issues for discussion:

  • It may not be obvious that code segments are loading since there aren't currently visual indications for this. Perhaps a loading icon can be displayed when code segments are still being loaded. However this can be a little tricky as of now due to the point below.
  • Loading of segments are currently done per-segment. Each file however can comprise of multiple segments, which each render independently. So, some segments may load earlier than others. While this allows for better responsiveness, it also can be a little confusing for chunks of code to load separately.
  • An ideal solution for lazy loading might involve promises of some kind to allow for loads of subsequent code segments (that are not yet visible) to be done in the background while other code is being viewed. This might involve significant refactoring of the way segments are rendered.
  • Doing the above would seem to naturally extend into ramps, which would also be worth making lazy. #1477 seems to have been closed due to issues with the spinner which I haven't been able to reliably observe with code segments. @gerhean Do you still happen to remember specifically the issue that was observed in #1477?

gok99 avatar Apr 18 '22 01:04 gok99

Doing the above would seem to naturally extend into ramps, which would also be worth making lazy. https://github.com/reposense/RepoSense/pull/1477 seems to have been closed due to issues with the spinner which I haven't been able to reliably observe with code segments. @gerhean Do you still happen to remember specifically the issue that was observed in https://github.com/reposense/RepoSense/pull/1477?

Spinner does not show up when list of repos is sorted

gerhean avatar Apr 18 '22 01:04 gerhean

Spinner does not show up when list of repos is sorted

I see, this doesn't seem to be an issue in the dashboard deployment above - the spinner seems to appear when files in authorship view are sorted, and when repos are sorted.

gok99 avatar Apr 18 '22 01:04 gok99

Actually I just found a regression from #1647 in which the code view is hidden for half the files, so lazy loading for authorship may not really do much at the moment. image

Loading of segments are currently done per-segment. Each file however can comprise of multiple segments, which each render independently. So, some segments may load earlier than other. While this allows for better responsiveness, it also can be a little confusing for chunks of code to load separately.

I prefer loading per file

gerhean avatar Apr 18 '22 01:04 gerhean

Actually I just found a regression from https://github.com/reposense/RepoSense/pull/1647 in which the code view is hidden for half the files, so lazy loading for authorship may not really do much at the moment.

Good point! Files were hidden beyond a character limit in #1647. However, the files to hide aren't re-computed on a sort, so they remain hidden. This PR should remove the need to impose an overall character limit, which should fix the regression.

gok99 avatar Apr 18 '22 03:04 gok99

Hi, We are going to mark this PR as stale because it has been inactive for the past 30 days. If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label. Do let us know if you are stuck so that we can help you!'

github-actions[bot] avatar Jul 10 '22 00:07 github-actions[bot]

@gok99 Any updates on this PR? Is there anything that you require help/clarifications with?

dcshzj avatar Jul 11 '22 03:07 dcshzj

@dcshzj I think the PR is about ready for a preliminary review. I'll take it out of draft.

With respect to my earlier comment, this PR doesn't include:

  • Loading indication (like a spinner or just the words 'loading..'). I'm not sure if this is really necessary but I can put it in if there's concern that there will be confusion for longer loading segments.
  • Background loading of segments that aren't currently visible. I don't think this is really necessary either since there isn't very much point loading segments that will possibly never be seen.

The PR also removes the overall character threshold which has caused various other issues (https://github.com/reposense/RepoSense/pull/1750#issuecomment-1101021415, https://github.com/reposense/RepoSense/issues/1772#issue-1226495513)

gok99 avatar Jul 11 '22 07:07 gok99

Hi, We are going to mark this PR as stale because it has been inactive for the past 30 days. If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label. Do let us know if you are stuck so that we can help you!'

github-actions[bot] avatar Aug 11 '22 00:08 github-actions[bot]

Requesting a review @gerhean @Zhou-Jiahao-1998

gok99 avatar Aug 11 '22 07:08 gok99

I think resolve the conflict first? Otherwise LGTM

gerhean avatar Aug 11 '22 09:08 gerhean

@gerhean oops, didn't notice the conflict on my phone. should be good to go now

gok99 avatar Aug 12 '22 02:08 gok99

The following links are for previewing this pull request:

  • Dashboard Preview: https://dashboard-1750-pr-reposense-reposense.surge.sh
  • Docs Preview: https://docs-1750-pr-reposense-reposense.surge.sh

github-actions[bot] avatar Aug 15 '22 02:08 github-actions[bot]