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

Y.js backend enhancements and debug helpers #4310

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Y.js backend enhancements and debug helpers #4310

merged 2 commits into from
Sep 13, 2023

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jun 19, 2023

  • chore: Add helpful comments and code bits for y.js debugging
  • chore: Basic backend y.js message decoder

📝 Summary

While looking into #4309 I thought it might be nicer to actually decode parts of the y.js message to perform relevant checks on it. This is now done in the YjsMessage class where so far only the relevant bits for extracting the message type is implemented. Upstream references are left in the code.

In addition I added a few logging parts in comments that might be useful in future debugging/development.

🏁 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
Copy link

cypress bot commented Jun 19, 2023

3 failed and 4 flaky tests on run #12081 ↗︎

3 137 18 0 Flakiness 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Y.js backend enhancements and debug helpers
Project: Text Commit: d7d78107e4
Status: Failed Duration: 29:01 💡
Started: Sep 11, 2023 3:30 PM Ended: Sep 11, 2023 3:59 PM

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

@juliusknorr

This comment was marked as outdated.

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.

Nice! The code looks good to me, but I'd prefer if we could get a second review - especially of readVarUint() in lib/YjsMessage.php 🤓

@juliusknorr
Copy link
Member Author

Conflicts resolved, @max-nextcloud any chance you find some time? Not urgent but trying to get my open pr count down a bit 🙈

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

The parsing of the base64 encoded messages and extraction of the first bits to get the types is nicely covered with tests. So I'm confident it's good as it is.

@max-nextcloud max-nextcloud merged commit 439e7e5 into main Sep 13, 2023
34 checks passed
@max-nextcloud max-nextcloud deleted the chore/yjs branch September 13, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants