RepoSense
RepoSense copied to clipboard
[#1748] Lazy loading for code segments in authorship view
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.
The relevant PR has been merged but there still seems to be merge conflicts?
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?
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
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.
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.
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
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.
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!'
@gok99 Any updates on this PR? Is there anything that you require help/clarifications with?
@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)
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!'
Requesting a review @gerhean @Zhou-Jiahao-1998
I think resolve the conflict first? Otherwise LGTM
@gerhean oops, didn't notice the conflict on my phone. should be good to go now
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