Confirm that the user really wants to leave with an unsubmitted MR review
What does this MR do and why?
For #213805 (closed)
Why
Some folks forget to submit a pending review ("batch comments") and leave the MR Diffs page without submitting it, sometimes leading to long MR review turnarounds as they have to be told they maybe forgot something, go back, and then re-send it to the author.
What
This MR adds a beforeunload
event listener when the state of the pending comments changes (e.g. it moves from no pending comments to pending comments).
The browser native behavior is the only available option in this scenario, so a generic "you have unsaved changes" message is presented.
Screenshots or screen recordings
Reload | Close (tab) | |
---|---|---|
Firefox | (no distinction from reload) | |
Chrome |
How to set up and validate locally
- Load an MR
- Go to the Changes tab
- Write a comment
- Click "Start a review" (instead of "Add comment now")
- Attempt to "unload" the page (close the browser, close the tab, navigate back, reload the tab, etc.)
- The browser alert should interrupt the unload until you confirm that you want to leave
Alternative:
- Load an MR (Changes) with pending review comments
- Interact with the page in any way (click on something, collapse a file, etc.)
- Attempt to "unload" the page (close the browser, close the tab, navigate back, reload the tab, etc.)
- The browser alert should interrupt the unload until you confirm that you want to leave
Caveats
There are a lot of caveats to this feature:
- Probably the most "breaking" caveat is that browsers will not show this dialog if the user does not interact with the page.
- For the initial creation of a pending review (e.g. the first set of validation steps above), everything will work as expected. However...
- If a user visits an MR that they had previously created a pending review for, but do not interact with the page, the browser will not block the "unload".
- This is why the alternative validation steps explicitly say to interact with the page.
- Another caveat is that we (the application) don't have any control over what this UI looks like.
- See the screenshots for examples of what this shows as.
- Older implementations would sometimes display the message we choose, but this has largely been abandoned in modern browsers - likely because of malicious usage by sites.
- We cannot show other messages (like a simple
alert
that says "You have unsubmitted review comments!") prior to halting the page unload due to malicious/annoying popup mechanisms in the past. - There are performance implications of preventing a page from unloading.
- This MR should handle them (by only binding when there are actually pending draft comments, rather than binding always and checking when the event is triggered), but it's still going to affect the performance of cached page navigations (e.g. bfcache).
- This may not work on mobile.
- I think that folks who think they want this feature will quickly grow to despise it. The UX here is very annoying.
😉
Some other caveats are technical:
- Until now, there's no link between the
batch_comments
Vuex store and thediffs
app.- The muddiness between what is
diffs
, what isnotes
, and what isbatch_comments
continues to deepen. - With this MR, I have tried to keep these two as separate as possible: They only communicate over a message channel, and it's up to the
diffs
app to subscribe (only) and it's up to thebatch_comments
app to broadcast when the status (there is a pending review or not) changes.
- The muddiness between what is
- We can't do more "robust" things (like the Gmail-style "no attachments" warning) without substantially rethinking our application architecture.
- The reason that's possible is because Gmail is a true SPA where most behaviors are handled by the client. We still rely very heavily on a server backend to handle many requests, so something like "sending an email without an attachment that was mentioned" (which would be something like "leaving the Diffs app without submitting your pending review" in our case) isn't something the front end application can usually halt: we depend heavily on full server round trips. In any case,
beforeunload
is the only way to halt the tab/browser closing, and it's very unreliable.
- The reason that's possible is because Gmail is a true SPA where most behaviors are handled by the client. We still rely very heavily on a server backend to handle many requests, so something like "sending an email without an attachment that was mentioned" (which would be something like "leaving the Diffs app without submitting your pending review" in our case) isn't something the front end application can usually halt: we depend heavily on full server round trips. In any case,
Other caveats relate directly to the implementation in this MR:
- I don't know of any way to make RSpec expect a confirmation dialog when the browser attempts to unload. I haven't added an integration test due to this, although I think that the JS tests are plenty to cover the scenario (they test that our code works, testing that the dialog appears is just testing that browsers work).
- In the tests for editing an MR (another place where leaving without saving triggers the unload dialog), we don't test any of the "failure" paths (also, there are two files for editing MRs
🤔 ).
- In the tests for editing an MR (another place where leaving without saving triggers the unload dialog), we don't test any of the "failure" paths (also, there are two files for editing MRs
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.