Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for partial loading or not loading at all of the GitHub Writer #348

Merged
merged 2 commits into from
Aug 12, 2022

Conversation

Comandeer
Copy link
Member

The GHW's router listened to pjax events to detect when it should destroy the old editors and initialize new ones. However, it seems that GH no longer uses the pjax and it uses Turbo instead. I've replaced the pjax events with their Turbo equivalents:

  • pjax:startturbo:before-visit,
  • pjax:endturbo:render.

Thanks to that, the navigation between pages is correctly detected and handled and GHW is once again loading correctly.

Closes #337.

@mlewand mlewand requested review from mlewand and Dumluregn August 9, 2022 10:43
@mlewand
Copy link
Contributor

mlewand commented Aug 9, 2022

@Dumluregn let's have it reviewed either by you or me.

@mateuszzagorski
Copy link
Contributor

mateuszzagorski commented Aug 10, 2022

I've tested this briefly, and can confirm that it is working way better now.
However there are still cases in which the editor does not load for me:
- When I'm opening PR for the first time, the comment editor at the bottom does not update. It starts to work after refreshing the page.
- Trying to edit any of the comments (works when you edit the same comment again).
- Creating new PR (works after refreshing).

@Comandeer
Copy link
Member Author

@mateuszzagorski, I can only reproduce the issue with editing comments. Indeed, the editor is then partially loaded – yet it doesn't seem to fix itself after editing the same comment again. However, I can't reproduce other issues you raise. Could you share some more details?

@mateuszzagorski
Copy link
Contributor

Sure @Comandeer. I am experiencing the issues when using Chrome (v 104.0.5112.79 ).
On Firefox (v 103.0.2) I was unable to replicate even the editing comments problem.

1. Opening PR for the first time:
- Go to the 'pull requests' tab  https://github.com/ckeditor/github-writer/pulls
- Click on any of the available PR's
- Scroll all the way down, and what I see is:

After refreshing it's working as expected.

2. Creating a new PR
- Go to the 'pull requests' tab -> https://github.com/ckeditor/github-writer/pulls and simply proceed with creating a new PR
And what I can see is:

Similarly to the previous issue - when refreshed, it's working as expected.

@Comandeer
Copy link
Member Author

I can't reproduce the issue with creating new PR but I'm able to reproduce the issue with navigating to PR. It's present only at the first navigation to the PR, then it disappears.

@Dumluregn
Copy link
Contributor

Dumluregn commented Aug 11, 2022

I conducted a research on how this PR improves the loading of GH Writer in different scenarios. 

  • The build was tested against the one from master branch. 
  • I've used dev version for both.
  • Tested both browsers (Chrome and FF), but there were no significant difference besides scenarios where FF doesn't work "by design" (CodeEditor).
  • Each test involved 10 page loads. I didn't refresh the page but went back and forth. 

The results are in the table below. To sum up:

  • PR doesn't break anything that was broken already 🎉
  • It greatly improves the loading when:
    • creating a new issue;
    • adding a new comment under the pull request (besides the very first visit);
    • adding a review comment;
    • commenting a commit;
    • editing the milestone description.
  • There are a few particular scenarios where GH Writer doesn't load - but it does it consistently, which means we should probably extract those cases and take a look at them separately.

I think that this PR is a great improvement on its own and it's worth to release a new version once we merge it.

(the table below has 5 columns - you may need to scroll it horizontally)

Tested item Exemplary URL Fail ratio Comment Worked before?
comment on the issue page #346 0   mostly yes
new issue https://github.com/ckeditor/github-writer/issues/new?assignees=&labels=bug&template=1-bug-report.md&title= 0   very bad, each time a new issue is created GHW doesn't load until refreshed
edit comment #347 (comment) 30% doesn't work only if you click "edit" button before the whole page is loaded (see zenhub or editor in new comment field) same as after change
new comment under pull request ckeditor/ckeditor5#12254 0   very bad, each time you visit a PR GHW doesn't load
new comment under pull request - first visit ckeditor/ckeditor5#12254 100% confirmed by Tomek and Mateusz - #348 (comment) no
new PR that can be automatically merged ckeditor/ckeditor5@master...ck/epic/11857-table-resize-improvements 0 even if clicked "Create pull request" before full page load yes
new PR that can't be automatically merged master...t/188 100% go to https://github.com/ckeditor/github-writer/compare, select t/188 and "Create pull request" no
new review comment https://github.com/ckeditor/ckeditor5/pull/12254/files 10% only the first time (at all, not at each PR) 50/50
edito non-markdown file https://github.com/ckeditor/github-writer/edit/master/.gitignore 100% only markdown files are supported - by design no
edit markdown file https://github.com/ckeditor/github-writer/edit/master/README.md 0 only markdown files are supported - by design yes
wiki page https://github.com/ckeditor/ckeditor5/wiki/Home/_edit 100%   no
saved replies https://github.com/settings/replies 100% looks like GHW tries to load and resigns no
commit comment ckeditor/ckeditor5@bcf3d44 0   worked 30% of times
milestone https://github.com/ckeditor/ckeditor5/milestones/55/edit 0   50/50
releases https://github.com/ckeditor/ckeditor5/releases/new 0   yes
edit saved reply https://github.com/settings/replies/81985/edit? 0   yes
release edit https://github.com/ckeditor/ckeditor5/releases/edit/v35.0.1 100%   no

@mlewand
Copy link
Contributor

mlewand commented Aug 12, 2022

Ok, since we have a confirmation that it indeed improves many cases - let's merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub Writer partially loaded
4 participants