Using mozreview offline is not so great

ASSIGNED
Assigned to

Status

MozReview
General
ASSIGNED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: Brian Birkhauser)

Tracking

(Blocks: 1 bug)

Details

User Story

From the diff views, users can leave comments by clicking on line numbers. When the first comment is left, a draft is created and saved on the server. It's published when the user clicks the "Publish Review" button (either on the banner at the top or from the "Edit Review" dialog).

The problem here is that the drafts are saved entirely on the server; there's no local cache. So if you can't reach the server, you'll get an error when the site tries to save the comment. You won't even be able to publish it when you get back online, and when you refresh the comments will be gone.

We'd like to fix this by using localStorage or similar to cache the review if the user is offline. Best would be for it to automatically save the draft to the server when it gets back online, or at least when the user tries to publish it (the draft has to exist on the server before the publish can succeed).

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

I tried doing a review while offline today (loaded the diff while I was online, of course).  It would let me add comments, but showed errors while trying to save them.  Then when I reconnected I had to go through all the comments and manually resave them all for them to get noticed, as far as I could tell.  Specifically, I added a comment after I was back online and checked the review status in a different tab: the new comment was there, but the old ones were not.

This is significantly less usable than a textarea with the diff text quoted in it, where I can just write comments, sessionstore will save them, and I can submit them when back online....

Comment 1

2 years ago
If this can't be fixed in mozreview, the option to move request to bmo could help here.
I assume this will become more a problem to me too during summer when I tend to spend more time in trains (and often doing reviews there).
Yes, the "do reviews while in transit" use case is the main use case for offline reviews, absolutely.

Comment 3

2 years ago
In theory it should be possible for RB to use a service worker that would save things such as comments when the browser is offline to retransmit them when there is a network connection, and also to save pages such as the diff viewer that are loaded while being offline for later offline viewing.

Updated

2 years ago
Assignee: nobody → brian.birkhauser
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

User can add, edit and delete diff comments while offline, and sync those comments once the user goes back online and performs any of the following actions: create new comment, edit a comment, publish review.

Review commit: https://reviewboard.mozilla.org/r/65868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65868/
Attachment #8773151 - Flags: review?(mcote)

Updated

2 years ago
Attachment #8773151 - Flags: review?(smacleod)

Updated

2 years ago
Attachment #8773151 - Flags: review?(glob)
Attachment #8773151 - Flags: review?(dwalsh)

Updated

2 years ago
User Story: (updated)
https://reviewboard.mozilla.org/r/65868/#review63998

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:1
(Diff revision 1)
> +/**

Jumping back and forth between double and single quotes -- we should stick to single quotes.

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:79
(Diff revision 1)
> +    var i, id, ids, len, ref, results, store, dirty;
> +    store = localStorage.getItem((this.getStoreName(this)) + "_dirty");
> +    ids = (store && store.split(',')) || [];
> +    results = [];
> +    dirty = [];
> +    for (i = 0, len = ids.length; i < len; i++) {

Can we use `_.each`  here?

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:122
(Diff revision 1)
> +  syncDestroyed(options) {
> +    var i, id, ids, len, model, results, store;
> +    store = localStorage.getItem((this.getStoreName(this)) + "_destroyed");
> +    ids = (store && store.split(',')) || [];
> +    results = [];
> +    for (i = 0, len = ids.length; i < len; i++) {

`_.each` here too?
https://reviewboard.mozilla.org/r/65868/#review64004

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:12
(Diff revision 1)
> + *  Any model needing offline support are expected to extend this.
> + * 
> + *  Based off of https://github.com/nilbus/Backbone.dualStorage
> + */
> +
> +window.modelUpdatedWithResponse = function(model, response) {

Can we dodge using `window`?  `RB`?

::: reviewboard/reviewboard/static/rb/js/resources/utils/localStore.js:1
(Diff revision 1)
> +'use strict';

Can we put `S4` and `Store` onto the `RB` object instead fo `window`?  `Store` seems like it could be implemented by the browser some day.
https://reviewboard.mozilla.org/r/65868/#review64130

Just did a quick high level pass, mostly found style issues.

::: reviewboard/reviewboard/static/rb/js/resources/models/baseCommentModel.js:138
(Diff revision 1)
> +        if(rsp.include_text_types) {
> +            types = rsp.include_text_types.split(',');
> +        }
> +        _.each(types, function(type) {
> +            var txt_field = type + '_text_fields';
> +            

The RB diff viewer will show blocks of red like this one where there is trailing whitespace - we should clean this up before landing (only opening this single issue to track all instances)

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:38
(Diff revision 1)
> +  return model;
> +};
> +
> +
> +RB.LocalResource = RB.BaseResource.extend({
> +	defaults() {

I think there is a tab mixed in here.

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:47
(Diff revision 1)
> +          
> +      }, RB.BaseResource.prototype.defaults());
> +  },
> +  offlineStatusCodes: [408, 502],
> +  hasTempId() {
> +		return _.isString(this.id) && this.id.length === 36 && this.id.indexOf('t') === 0;

Looks like more tabs mixed in - we'll want to clean all of the tab characters up so it's spaces only. I'll avoid marking anymore of these and just have this one issue to track it.

::: reviewboard/reviewboard/static/rb/js/resources/utils/localStore.js:1
(Diff revision 1)
> +'use strict';

This file could use some documentation, at least an overview of the data stored and what it represents.
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review64028

::: reviewboard/reviewboard/static/rb/js/resources/utils/localStore.js:114
(Diff revision 1)
> +
> +  Store.prototype.findAll = function () {
> +    var i, id, len, ref, results;
> +    ref = this.records;
> +    results = [];
> +    for (i = 0, len = ref.length; i < len; i++) {

Can we use `Array.prototype.map` instead of `for` loop and `results` array?
Attachment #8773151 - Flags: review?(dwalsh) → review-
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review64300

Clearing my review flag until next update.
Attachment #8773151 - Flags: review?(smacleod)
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review64476
Attachment #8773151 - Flags: review?(mcote)
(Assignee)

Comment 11

2 years ago
Created attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

Review commit: https://reviewboard.mozilla.org/r/67524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67524/
Attachment #8775355 - Flags: review?(smacleod)
Attachment #8775355 - Flags: review?(mcote)
Attachment #8775355 - Flags: review?(dwalsh)
https://reviewboard.mozilla.org/r/67524/#review64786

FYI, it is better if you amend your old commit rather than pushing a fix-up commit.  We like to review patches as they are intended to land, i.e., without squashing.  Mercurial will preserve history so that we get useful interdiffs and such.
Attachment #8773151 - Flags: review?(glob)
https://reviewboard.mozilla.org/r/67524/#review65086

Thank you for the work, Brian!  I can't wait to get this stuff in!

One problem I saw was after the following test:

1.  Load a diff page
2.  Make a comment while online
3.  Go offline, make a few comments
4.  Go back online
5.  Click Publish button

The review dialog will come up but the offline comments aren't added to the comment listing within the dialog.  I needed to add another comment while online for the offline comments to then display in the review dialog.

Additionally, if I refresh the page, comments made while offline are still in localStorage but they don't display in the diff upon page load.  

These two issues will lead to confusion about what's been "saved" and what to do with it.

Thinking out loud, it may be worth attempting to send those comments to server when the `ReviewDialogView` instance is created -- that will hopefully ensure the offline comments are there when the dialog loads.  It may also be a good idea to poll for online connection at a given interval to send those comments to server.
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review65342
Attachment #8775355 - Flags: review?(dwalsh) → review-
(Assignee)

Comment 15

2 years ago
(In reply to David Walsh :davidwalsh from comment #13)
> https://reviewboard.mozilla.org/r/67524/#review65086
> 
> Thank you for the work, Brian!  I can't wait to get this stuff in!
> 
> One problem I saw was after the following test:
> 
> 1.  Load a diff page
> 2.  Make a comment while online
> 3.  Go offline, make a few comments
> 4.  Go back online
> 5.  Click Publish button
> 
> The review dialog will come up but the offline comments aren't added to the
> comment listing within the dialog.  I needed to add another comment while
> online for the offline comments to then display in the review dialog.
> 
> Additionally, if I refresh the page, comments made while offline are still
> in localStorage but they don't display in the diff upon page load.  
> 
> These two issues will lead to confusion about what's been "saved" and what
> to do with it.
> 
> Thinking out loud, it may be worth attempting to send those comments to
> server when the `ReviewDialogView` instance is created -- that will
> hopefully ensure the offline comments are there when the dialog loads.  It
> may also be a good idea to poll for online connection at a given interval to
> send those comments to server.


I'm attempting to ensure the comments are saved. I already have code in that will save on publish. However, I am trying to get it to save on reload.  I'm trying to track down the best place to put the call.  ReviewDialogView doesn't work. I have a call in ReviewablePageView, which works, but you have to refresh again to see the added comments. I can't figure out how to reload the comments or add them to the view.  any thoughts?
Hey Brian, apologies for the delay -- loads going on here.  

Within `reviewDialogView_mozreview.js`, there's a BaseCommentView resource that pulls in and saves comments so I think that's the best bet to look at!
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review66150
Attachment #8775355 - Flags: review?(mcote)
(Assignee)

Comment 18

2 years ago
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67524/diff/1-2/
Attachment #8775355 - Attachment description: reviewboard: fixed style issues, for loops, changed es6 code, added comments to localStore.js. Changed references from window to RB. (bug 1259237); → reviewboard: added code to ensure local comments are saved on refresh and added to the view. (bug 1259237);
Attachment #8775355 - Flags: review?(mcote)
Attachment #8775355 - Flags: review?(dwalsh)
Attachment #8775355 - Flags: review-
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

Hm not sure why this wasn't cleared when I cleared it in MozReview...
Attachment #8775355 - Flags: review?(mcote)
(Assignee)

Comment 20

2 years ago
I pushed a change that ensures comments that are stored locally will appear on refresh.

Comment 21

2 years ago
mozreview-review
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review67802

Yes yes

Comment 22

2 years ago
mozreview-review-reply
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review67802

Sorry, this was a test -- please disregard :)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review68424

I tested as follows:

1.  Make 2 comments while online
2.  Go offline, make 2 more comments
3.  Come back online, make another comment
4.  Click "Finish Review"
4.5 See 5 comments there -- yay!
5.  Refresh the page
6.  See 5 comment icons in the diff -- yay!
7.  Click "Finish Review" 
8.  See 5 comments there -- yay!

At a basic level things look like they're working, but I'll be continuing testing on this.

In the mean time, could you please rebase this on what's currently "public"?  We've merged some stuff since then that may cause conflict so we should hash that out ASAP.

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:305
(Diff revision 2)
> -     *
> +   *
> -     *     context (object):
> +   *     context (object):
> -     *         Context to bind when executing callbacks.
> +   *         Context to bind when executing callbacks.
> -     */
> +   */
> -    destroy(options={}, context=undefined) {  
> +  destroy: function(options, context) {
> +    if(!options) {

```
options = options || {}
```

::: reviewboard/reviewboard/static/rb/js/resources/models/localResourceModel.js:387
(Diff revision 2)
>     *
>     *     context (object):
>     *         Context to bind when executing callbacks.
>     */
> -    fetch(options={}, context=undefined) {
> +  fetch: function(options, context) {
> +    if(!options) {

```
options = options || {};
```

Comment 24

2 years ago
mozreview-review
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review69888

Marking as r- because changes needed.
Attachment #8775355 - Flags: review?(dwalsh) → review-

Comment 25

2 years ago
mozreview-review
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review69890

While you rebase, Brian, one more important note when committing to the fork.

Whenever modifying a core RB file, like a template .html or a .js file, we first need a single commit which:

1.  Creates a copy of the file called `{original_name}_mozreview.js|html`
2.  Provides that file the same content as the original
3.  Updates the file from which we're pulling from to be the `_mozreview` file.

An example of this can be found here:  https://reviewboard.mozilla.org/r/69634/diff/3#index_header

Doing this will make future upgrades to the fork easier.

Thank you!

Comment 26

2 years ago
mozreview-review
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65866/#review74028

Hey Brian!

Please structure your 2 commits as follows:

1.  Create one commit where you copy core JS files you'll be changing (`diffReviewableModel.js`, `localResourceModel.js`, `localStore.js`) whereby you add `_mozreview` to the file name.  For example, copy `diffReviewableModel.js` to a file called `diffReviewableModel_mozreview.js`, but don't make any changes to the contents yet.  Then update the `staticbundles.py` file with the updated file reference (i.e. change `diffReviewableModel.js` to `diffReviewableModel_mozreview.js`
2.  Make your code updates to the `_mozreview.js` files

Please let me know if you have any questions!

David
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review75460

A nit: you should follow the same commit message structure here as we use in other commits in http://hg.mozilla.org/webtools/reviewboard/, that is, "MozReview: <summary> (bug <bug id>). r?<reviewer>"

<reviewer> should be davidwalsh here, as he'll be doing the main review.
Attachment #8773151 - Flags: review?(mcote) → review-

Comment 30

2 years ago
mozreview-review
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review75462

As with the other commit, please follow the standard commit message format.  Also just flag davidwalsh for review; I'm only overseeing this from a high level. :)
Attachment #8775355 - Flags: review?(mcote) → review-

Comment 31

2 years ago
mozreview-review
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review75476

You nailed the `_mozreview.js` concept, nice!  The `staticbundles.py` update should also be in this commit though.  I see you put it in the second commit so a quick move over to this first commit would be great! :)
Attachment #8773151 - Flags: review?(dwalsh) → review-

Comment 32

2 years ago
mozreview-review
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review75494

A few minor things I found quickly.  Will do manual testing after this.

::: reviewboard/reviewboard/static/rb/js/resources/models/baseCommentModel_mozreview.js:142
(Diff revision 3)
> +            var txt_field = type + '_text_fields';
> +            if(!rsp[txt_field]) {
> +                //should do some better parsing like in mix_ins.py
> +                data[txt_field] = {
> +                    text: rsp.text
> +                }

semicolon

::: reviewboard/reviewboard/static/rb/js/resources/models/diffCommentModel_mozreview.js:84
(Diff revision 3)
>      /*
> +     * Adds some of the data that the server normally responds with, while in offline mode
> +     */
> +    parseOfflineData: function(rsp) {
> +        var result = parentProto.parseOfflineData.call(this, rsp);
> +      

whitespace
Attachment #8775355 - Flags: review?(dwalsh) → review-

Comment 33

2 years ago
mozreview-review
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review75532

Manual testing here didn't work;  I made one comment online, two offline, and when I go back online, click "Edit Review" in the header, the dropdown dialog loads but only the online comment is populated.  Possibly there was a rebase error with the new dropdown dialog work I completed?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

2 years ago
mozreview-review
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review76278

Perfect!
Attachment #8773151 - Flags: review?(dwalsh) → review+

Comment 37

2 years ago
mozreview-review
Comment on attachment 8775355 [details]
MozReview: Added changes that fix bug 1259237, combined in an attempt to move commits into file structure that is more suitable for merging and keeping track of mozreview specific files. Fixed issue with edit review dropdown not showing offline diff comme

https://reviewboard.mozilla.org/r/67524/#review76296

I gave this a test today and things are still a bit off.  I did the following:

1.  Loaded two separate commit diffs in separate tabs.
2.  Used Chrome's Network tab to go offline
3.  Made 3 offline comments on both commits
4.  Went back online
5.  Clicked "Edit Review"
6.  The comments I made offline are not in the dropdown (problem).  I *do* see, however, that there is comment content in localStorage.
7.  I click "Finished Edit", then "Edit Review" again -- comments still not loading in the dialog yet.

Maybe there's a race condition?
Attachment #8775355 - Flags: review?(dwalsh) → review-

Comment 38

2 years ago
mozreview-review
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review76476
Attachment #8773151 - Flags: review?(mcote)

Updated

2 years ago
Blocks: 1301169
Attachment #8773151 - Flags: review?(smacleod)

Comment 39

2 years ago
mozreview-review
Comment on attachment 8773151 [details]
MozReview: Created copies of files to be edited. (Bug 1259237).

https://reviewboard.mozilla.org/r/65868/#review83998
Attachment #8773151 - Flags: review?(glob)
You need to log in before you can comment on or make changes to this bug.