Skip to content

Conversation

@max-nextcloud
Copy link
Collaborator

@max-nextcloud max-nextcloud commented Jul 12, 2023

📝 Summary

When opening the same file over and over again in the page-links test in collectives the file turns empty after some time.
This is due to the file getting saved before it's even been loaded. So it will be saved empty.

This fix is more broad then that. It will ensure files are only saved during close if any undoable changes have been made to them - the same logic we also apply on autosave.

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests -> this was discovered due to cypress tests failing in collectives.
  • Documentation is not required

@cypress
Copy link

cypress bot commented Jul 12, 2023

1 failed tests on run #11019 ↗︎

1 147 2 0 Flakiness 0

Details:

fix(sync): only save on close if changes were made
Project: Text Commit: c6f6d1d4c9
Status: Failed Duration: 03:59 💡
Started: Jul 12, 2023 9:00 PM Ended: Jul 12, 2023 9:04 PM
Failed  cypress/e2e/sync.spec.js • 1 failed test

View Output Video

Test Artifacts
Sync > recovers from a lost connection Output Screenshots

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

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Very nice catch 🎣

@mejo-
Copy link
Member

mejo- commented Jul 12, 2023

/compile

Signed-off-by: nextcloud-command <[email protected]>
unsubscribe('text:image-node:delete', this.onDeleteImageNode)
if (this.dirty) {
const timeout = new Promise((resolve) => setTimeout(resolve, 2000))
await Promise.any([timeout, this.$syncService.save()])
Copy link
Member

@juliusknorr juliusknorr Jul 12, 2023

Choose a reason for hiding this comment

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

Maybe I'm missing something but wouldn't this be called after the text session has been closed already then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. The text session is closed afterwards where we destroy all the providers - one of which is the SyncServiceProvider which will close the session on destruction.
Only if the save takes longer than the timeout the provider will be destroyed never the less.

Copy link
Member

Choose a reason for hiding this comment

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

Right, makes sense. I just got confused by the close/_close methods of the sync service and how they would be triggered as I assumed that the viewer close calls them, but that doesn't seem to be the case. 👍

@juliusknorr juliusknorr added bug Something isn't working 3. to review high labels Jul 12, 2023
@juliusknorr
Copy link
Member

Failure is unrelated

@juliusknorr juliusknorr merged commit 3b71af8 into main Jul 12, 2023
@juliusknorr juliusknorr deleted the fix/only-save-with-changes branch July 12, 2023 22:09
@juliusknorr
Copy link
Member

/backport 0fe2f30 to stable27

@juliusknorr
Copy link
Member

/backport 0fe2f30 to stable26

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 high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exiting a text file delayed

5 participants