Skip to content

fix: only load editor once even if connection is recovered - #3868

Merged
juliusknorr merged 2 commits into
mainfrom
fix/3785-just-one-editor
Mar 2, 2023
Merged

fix: only load editor once even if connection is recovered#3868
juliusknorr merged 2 commits into
mainfrom
fix/3785-just-one-editor

Conversation

@max-nextcloud

@max-nextcloud max-nextcloud commented Mar 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

📝 Summary

Only load the editor if it has not been loaded yet.
Move createEditor out of the onLoaded function.

onLoaded may be called multiple times.
When the connection to the server is lost and restored the collaboration extension reloads the websocket polyfill which in turn opens the SyncService
which will trigger the onCreated and onLoaded callbacks.

🚧 TODO

When loosing the connection the yjs websocket provider will try to reconnect automatically
However we still show the reconnect button. The automatic reconnection succeeded for me - but the button fails.

  • Hide the reconnect button in that state
  • Properly disconnect after a timeout and show reconnect dialogue then
  • Make sure the reconnect actually works

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Documentation is not required
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests

@max-nextcloud

max-nextcloud commented Mar 2, 2023

Copy link
Copy Markdown
Collaborator Author

This PR includes a test that cancels some requests to simulate a connection loss.
I was able to trigger the same behavior locally by stopping the nextcloud-docker-dev environment and restarting it a few seconds later. That way it's probably easier to explore this in the browser oneself.

@max-nextcloud

max-nextcloud commented Mar 2, 2023

Copy link
Copy Markdown
Collaborator Author

We will need to work on the TODO items listed above. However I think it would still be good to land this today before the RC to mitigate some of the problems we are seeing on the server right now.

@cypress

cypress Bot commented Mar 2, 2023 •
edited
Loading

Copy link
Copy Markdown

1 flaky tests on run #8812 ↗︎

0 139 0 0 Flakiness 1

Details:

fix: only load editor once even if connection is recovered
Project: Text Commit: 9192cda6f0
Status: Passed Duration: 03:31 💡
Started: Mar 2, 2023 12:00 PM Ended: Mar 2, 2023 12:03 PM
Flakiness  cypress/e2e/share.spec.js • 1 flaky test

View Output Video

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

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

juliusknorr
juliusknorr reviewed Mar 2, 2023
View reviewed changes
Comment thread src/services/SyncService.js Outdated
@max-nextcloud
max-nextcloud force-pushed the fix/3785-just-one-editor branch 2 times, most recently from 5c47bb5 to eb196e0 Compare March 2, 2023 10:39
@max-nextcloud

max-nextcloud commented Mar 2, 2023

Copy link
Copy Markdown
Collaborator Author

Okay... found a much simpler way of basically achieving the same.

One issue remains though:
After the automatic session recovery the old session will still be used for the mention extension.
Therefore mentions will not work anymore.

@juliusknorr

juliusknorr commented Mar 2, 2023

Copy link
Copy Markdown
Member

/rebase

@max-nextcloud @juliusknorr
fix: only load editor once even if connection is recovered
32eeb94 
`onLoaded` may be called multiple times.
When the connection to the server is lost and restored
the collaboration extension reloads the websocket polyfill
which in turn opens the SyncService
which will trigger the `onCreated` and `onLoaded` callbacks.

Prevent duplicate editor the simple way
- by checking if it exists already and only loading if it does not.

Signed-off-by: Max <[email protected]>
@nextcloud-command
nextcloud-command force-pushed the fix/3785-just-one-editor branch from eb196e0 to 32eeb94 Compare March 2, 2023 11:43
@juliusknorr

juliusknorr commented Mar 2, 2023

Copy link
Copy Markdown
Member

/compile

@nextcloud-command
Compile assets
9192cda 
Signed-off-by: nextcloud-command <[email protected]>
mejo-
mejo- approved these changes Mar 2, 2023
View reviewed changes

@mejo- mejo- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code looks good to me 👌

I don't fully understand the impact of moving up the initialization of this.backend above the emits.

@juliusknorr
juliusknorr merged commit 68c8268 into main Mar 2, 2023
@delete-merged-branch
delete-merged-branch Bot deleted the fix/3785-just-one-editor branch March 2, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes
@mejo- mejo- mejo- approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Prevent awareness message avalanche

4 participants

@max-nextcloud @juliusknorr @mejo- @nextcloud-command