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

feat: Use notify push for sync messages during editing #4585

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jul 26, 2023

Signed-off-by: Julius Härtl [email protected]

📝 Summary

This is a first PoC to make use of notify_push for syncing steps back to other editors instead of regular polling. We still poll every 30 seconds as a fallback and to ensure that the autosave requests are still triggered until #4424 is there (even with that some fallback sync might still be worth).

Can be manually enabled for now with

occ config:app:set text notify_push --type boolean --value=1

🚧 TODO

  • More manual testing in different scenarios
    • Check what happens with the base doc changing externally (did we use the sync to detect the conflict?)

🏁 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
  • Documentation (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Jul 26, 2023

1 flaky tests on run #11336 ↗︎

0 149 2 0 Flakiness 1

Details:

feat: Use notify push for sync messages during editing
Project: Text Commit: 90c956236b
Status: Passed Duration: 04:17 💡
Started: Jul 27, 2023 6:27 PM Ended: Jul 27, 2023 6:32 PM
Flakiness  cypress/e2e/nodes/Links.spec.js • 1 flaky test

View Output Video

Test Artifacts
... > Link website with selection Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Jul 28, 2023

Thanks a lot for looking into this. I'm really curious to play with it. 😍

We still poll every 30 seconds as a fallback and to ensure that the autosave requests are still triggered until #4424 is there (even with that some fallback sync might still be worth).

Just one remark. Autosave is independent from the polling backend these days. It's triggered by

this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL)

Never the less I agree that having a fallback for a start might still be worthwhile.

@juliusknorr juliusknorr force-pushed the feat/notify_push branch 3 times, most recently from 576e874 to eed2413 Compare March 15, 2024 14:53
@juliusknorr juliusknorr force-pushed the feat/notify_push branch 2 times, most recently from f4a7d2b to 69fc3f4 Compare April 18, 2024 06:46
lib/Service/ApiService.php Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member Author

This is good from my side. I did quite some testing locally with different scenarios of browsers going offline, coming back, overwriting the file externally and it seems to be quite smooth.

Nevertheless I added a feature flag to opt-in for now, so we can maybe testrun this a bit more on a daily instance.

@juliusknorr juliusknorr marked this pull request as ready for review June 13, 2024 14:53
Copy link
Contributor

@elzody elzody left a comment

Choose a reason for hiding this comment

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

I did some more reading on the notify_push app and how it's used, and with my basic understanding of the app and websockets, it looks okay to me. I will approve in case it counts here :)

@juliusknorr
Copy link
Member Author

juliusknorr commented Jun 25, 2024

  • Need to do an extra check for public share links (as I haven't yet)

@juliusknorr juliusknorr added this to the Nextcloud 30 milestone Jul 18, 2024
@juliusknorr juliusknorr force-pushed the feat/notify_push branch 2 times, most recently from ca024ac to f352174 Compare July 18, 2024 20:02
@juliusknorr juliusknorr merged commit 5ac996e into main Jul 19, 2024
61 of 64 checks passed
@juliusknorr juliusknorr deleted the feat/notify_push branch July 19, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using notify_push for syncing
4 participants