Skip to content

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

Merged
juliusknorr merged 2 commits into
mainfrom
feat/notify_push
Jul 19, 2024
Merged

feat: Use notify push for sync messages during editing#4585
juliusknorr merged 2 commits into
mainfrom
feat/notify_push

Conversation

@juliusknorr

@juliusknorr juliusknorr commented Jul 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Signed-off-by: Julius Härtl jus@bitgrid.net

📝 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

cypress Bot commented Jul 26, 2023 •
edited
Loading

Copy link
Copy Markdown

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
max-nextcloud reviewed Jul 28, 2023
View reviewed changes
Comment thread lib/Service/ApiService.php Outdated
@max-nextcloud

max-nextcloud commented Jul 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

text/src/services/SyncService.js

Line 91 in 14d29b1

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
mejo-
mejo- reviewed Jun 13, 2024
View reviewed changes
Comment thread lib/Service/ApiService.php Outdated
@juliusknorr

juliusknorr commented Jun 13, 2024

Copy link
Copy Markdown
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
elzody
elzody approved these changes Jun 13, 2024
View reviewed changes

@elzody elzody left a comment

Copy link
Copy Markdown
Contributor

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

juliusknorr commented Jun 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author
  • Need to do an extra check for public share links (as I haven't yet)

@juliusknorr
feat: Use notify push for sync messages during editing
fe164c2 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@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
feat: Add feature flag for notify push to test on nightly more easily
f468541 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr merged commit 5ac996e into main Jul 19, 2024
@juliusknorr juliusknorr deleted the feat/notify_push branch July 19, 2024 09:36
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elzody elzody elzody approved these changes

@mejo- mejo- Awaiting requested review from mejo- mejo- is a code owner

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud max-nextcloud is a code owner

@hweihwang hweihwang Awaiting requested review from hweihwang

Assignees

No one assigned

Labels

3. to review enhancement New feature or request performance 🚀

Projects

None yet

Milestone

Nextcloud 30

Development

Successfully merging this pull request may close these issues.

Using notify_push for syncing

4 participants

@juliusknorr @max-nextcloud @mejo- @elzody