Skip to content

Conversation

@mejo-
Copy link
Member

@mejo- mejo- commented Dec 18, 2024

📝 Summary

This will send a final save request on unsaved changes via the browsers native navigator.sendBeacon() function when navigating away from the website or the tab/browser is closed.

Implementation details:

  • While beforeunload event is less reliable than visibilitychange according to different sources on the internet, tests in Firefox on Linux desktop revealed that visibilitychange event doesn't fire when opening a new website in the same tab. beforeunload on the other hand reliably worked when switching web page and closing tab/browser.
  • We add and remove the beforeunload event with every dirty state change because according to MDN documentation websites with a registered beforeunload event don't benefit from bfcache optimization. See https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

Fixes: #6606

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests

@mejo- mejo- added bug Something isn't working 3. to review labels Dec 18, 2024
@mejo- mejo- requested a review from juliusknorr December 18, 2024 13:38
@mejo- mejo- self-assigned this Dec 18, 2024
@mejo- mejo- requested a review from max-nextcloud as a code owner December 18, 2024 13:38
@codecov
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 34.09091% with 29 lines in your changes missing coverage. Please review.

Project coverage is 58.76%. Comparing base (bcb75ee) to head (1df2abd).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/services/SessionApi.js 5.26% 18 Missing ⚠️
src/services/SyncService.js 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6798      +/-   ##
==========================================
- Coverage   58.79%   58.76%   -0.03%     
==========================================
  Files         477      477              
  Lines       35855    35885      +30     
  Branches      988      988              
==========================================
+ Hits        21080    21089       +9     
- Misses      14670    14691      +21     
  Partials      105      105              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@max-nextcloud max-nextcloud self-assigned this Dec 19, 2024
@max-nextcloud max-nextcloud force-pushed the feat/send_beacon_on_close branch 2 times, most recently from 009a095 to 2fa0402 Compare February 5, 2025 11:13
@max-nextcloud max-nextcloud force-pushed the feat/send_beacon_on_close branch from 2fa0402 to 215b87b Compare March 14, 2025 15:21
@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Mar 17, 2025

I will try it out this next week and see if my comments are actually true and fix them if needed.

@mejo- mejo- force-pushed the feat/send_beacon_on_close branch from 215b87b to 5a9e80e Compare April 1, 2025 14:47
@mejo-
Copy link
Member Author

mejo- commented Apr 1, 2025

A good way to test this is to set a debugger breakpoint (e.g. in saveBeforeUnload()) and type window.location('https://example.org') in the developer console. That's way more handy than closing the tab/browser. Credits for this nice little trick go to @juliusknorr 😉

@mejo- mejo- force-pushed the feat/send_beacon_on_close branch 3 times, most recently from bcb0bc3 to d7482df Compare April 2, 2025 12:21
@mejo-
Copy link
Member Author

mejo- commented Apr 2, 2025

/backport to stable31

@mejo-
Copy link
Member Author

mejo- commented Apr 2, 2025

/backport to stable30

@mejo- mejo- requested a review from max-nextcloud April 2, 2025 12:44
mejo- added 2 commits April 2, 2025 15:46
It used to check if the last saved version of the document is older than
the current version. But `document.currentVersion` no longer exists in
the backend response, so this didn't work since quite some time.

Signed-off-by: Jonas <[email protected]>
This will send a final save request on unsaved changes via the browsers
native `navigator.sendBeacon()` function when navigating away from the
website or the tab/browser is closed.

Fixes: #6606

Implementation details:
* While `beforeunload` event is less reliable than `visibilitychange`
  according to different sources on the internet, tests in Firefox on
  Linux desktop revealed that `visibilitychange` event doesn't fire when
  opening a new website in the same tab. `beforeunload` on the other
  hand reliably worked when switching web page and closing tab/browser.
* We add and remove the `beforeunload` event with every `dirty` state
  change because according to MDN documentation websites with a
  registered `beforeunload` event don't benefit from bfcache optimization.
  See https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- force-pushed the feat/send_beacon_on_close branch from d7482df to 1df2abd Compare April 2, 2025 13:47
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now.

@mejo- mejo- merged commit 4e486c7 into main Apr 2, 2025
62 of 64 checks passed
@mejo- mejo- deleted the feat/send_beacon_on_close branch April 2, 2025 14:03
@github-project-automation github-project-automation bot moved this from 👀 In review to ☑️ Done in 📝 Office team Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Block closing/navigating away with unsaved changes

3 participants