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

fix(sync): Save even if versions match #4286

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

max-nextcloud
Copy link
Collaborator

📝 Summary

Fixes #3404.

Apply saves even if the client and server version match.

The client version only reflects the steps which the client has received back from the server.
It may leave out the steps the client just send itself.

So if the versions match - save the file to be sure to include changes from the client.

🏁 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 is not required

@max-nextcloud
Copy link
Collaborator Author

I'm actually not sure this is the right approach.
Alternatively we could also count the version number up on the client side when we push changes that directly apply as we know we're synced to that point.

One downside of this might be that it could lead to saving files that have not been changed at all. However we now prevent this by checking the editor history before saving in the first place. Plus some of the automated changes also triggered changes to the text data structure which resulted in steps being pushed version incremented and unchanged files getting saved.

So the current approach is

  1. save only if there are undoable changes in the editor history
  2. save even if versions match

This should prevent both... data loss due to missing saves as well as changed files without actual user changes.

@cypress
Copy link

cypress bot commented Jun 12, 2023

2 flaky tests on run #10274 ↗︎

0 147 1 0 Flakiness 2

Details:

fix(sync): Save even if versions match
Project: Text Commit: 6aa598bb18
Status: Passed Duration: 03:54 💡
Started: Jun 19, 2023 7:20 PM Ended: Jun 19, 2023 7:24 PM
Flakiness  sync.spec.js • 1 flaky test

View Output Video

Test Artifacts
Sync > recovers from a lost connection Output Screenshots
Flakiness  api/UsersApi.spec.js • 1 flaky test

View Output Video

Test Artifacts
The user mention API > fetches users with valid session Output Screenshots

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

@juliushaertl
Copy link
Member

Alternatively we could also count the version number up on the client side when we push changes that directly apply as we know we're synced to that point.

Let's consider this as a possible follow up.

As discussed previously this might cover at least the cases where the push happened but the sync would be still pending. In the long term we may just split out the save.

Fixes #3404.

Apply saves even if the client and server version match.

The client version only reflects the steps which the client
has received back from the server.
It may leave out the steps the client just send itself.

So if the versions match - save the file to be sure to include changes from the client.

Signed-off-by: Max <max@nextcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File might not be saved if close is happening too fast
2 participants