-
Notifications
You must be signed in to change notification settings - Fork 2
fix/client resync mechanism & server persist leader & quality-of-life improvements #29
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements client resync mechanisms, server persistent leader functionality, and several quality-of-life improvements to enhance reliability and testing capabilities.
Key changes:
- Client auto-resync when updates aren't acknowledged within 3 seconds
- Server persistent leader using Redis-based distributed locking to prevent duplicate
persistDoc
calls - Allow custom socket instance injection for testing purposes
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
src/y-socket-io/y-socket-io.js | Implements persistent leader mechanism, client resync support, and error handling improvements |
src/y-socket-io/client.js | Adds update acknowledgment with timeout and retry logic for client resync |
src/server.js | Removes logging import and improves server shutdown handling |
src/api.js | Fixes Redis import to use named imports instead of namespace import |
package.json | Updates yjs dependency version |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
async registerPersistentLeaderHeartbeat () { | ||
this.persistentHeartbeatTimeout = setTimeout(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The method name 'register' is misleading since it's recursively scheduling itself. Consider renaming to 'scheduleHeartbeat' or 'runHeartbeatCycle' to better reflect its behavior.
Copilot uses AI. Check for mistakes.
// @ts-ignore | ||
formData.append('ydoc', new Blob([Y.encodeStateAsUpdateV2(ydoc)])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The @ts-ignore comment lacks explanation. Consider adding a comment explaining why the type checking is being suppressed or fix the underlying type issue.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
This PR includes:
persistDoc
is only called once per namespace.socket
instance to the client providertsup
broken due to incorrect Redis import