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

enh: store awareness messages separately #3776

Merged
merged 5 commits into from
Feb 9, 2023

Conversation

max-nextcloud
Copy link
Collaborator

@max-nextcloud max-nextcloud commented Feb 7, 2023

📝 Summary

🚧 TODO

  • fix migration

🏁 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

@max-nextcloud max-nextcloud force-pushed the enh/separate-awareness-messages branch from 352eef4 to 8b668d2 Compare February 7, 2023 15:17
@cypress
Copy link

cypress bot commented Feb 7, 2023

1 flaky tests on run #8567 ↗︎

0 132 0 0 Flakiness 1

Details:

enh: store awareness messages separately
Project: Text Commit: e89201fe51
Status: Passed Duration: 03:51 💡
Started: Feb 9, 2023 4:20 PM Ended: Feb 9, 2023 4:23 PM
Flakiness  cypress/e2e/share.spec.js • 1 flaky test

View Output Video

Test
Open test.md in viewer > Share a file with download disabled shows an error Screenshot

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

syncService.sendSteps(() => {
steps = this.#queue.map(s => encodeArrayBuffer(s))
outbox = this.#queue
Copy link
Member

Choose a reason for hiding this comment

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

When testing it seems that debouncing needs to be improved now for the push request, it seems on my test with two browsers the push is triggering every 200ms through the following stack trace even if the cursor positions do not change:

Screenshot 2023-02-08 at 12 10 59

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suspect this is due to us replaying old awareness messages and creating an inconstent state.
I created #3785 to track it.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud force-pushed the enh/separate-awareness-messages branch from 8b668d2 to 88b6fa7 Compare February 9, 2023 10:23
@max-nextcloud max-nextcloud marked this pull request as ready for review February 9, 2023 10:25
@max-nextcloud
Copy link
Collaborator Author

/compile

* `push` now expects an `awareness` parameter.
* Closing the window closes the session
  which causes new sessions to receive the content of the doc
  rather than the state.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud force-pushed the enh/separate-awareness-messages branch from 851bdc8 to ffaf7bb Compare February 9, 2023 15:28
@max-nextcloud
Copy link
Collaborator Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

👍

Also tested to collaborate a bit with @miaulalala and no collaboration issues encountered 🚀

@juliusknorr juliusknorr merged commit ee9ab9e into main Feb 9, 2023
@delete-merged-branch delete-merged-branch bot deleted the enh/separate-awareness-messages branch February 9, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only store last awareness messages on the backend
3 participants