Implement events for sync status #353

paulgb · 2024-12-11T02:54:32Z

y-websocket has a synced flag, but it is a bit misleading because it only tells whether the initial handshake was completed. Knowing whether a document was synced in general for y-protocols is a bit tricky, because we would need to compare the state vector and delete set.

When using WebSockets, we can take advantage of the ordering guarantee and synchronous message processing in Y-Sweet to implement a very simple check of sync.

The client stores two numbers, lastSyncSent and lastSyncAcked. When it sends an update to the server, it also increments lastSyncSent and then sends a separate messageSyncStatus message containing that number as the payload.

When the server receives a messageSyncStatus, it simply echoes it verbatim. When the client receives the messageSyncStatus in return, it updates lastSyncAcked to the payload of the message.

If lastSyncAcked = lastSyncSent, we know that all messages since the last update have been processed. Otherwise, there are outstanding changes.

This builds on #352 and will enable #306.

Demo:

Screen.Recording.2024-12-12.at.4.26.31.PM.mov

vercel · 2024-12-11T02:54:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
y-sweet-demos	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 12, 2024 10:37pm

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
y-sweet-debugger	⬜️ Ignored (Inspect)	Visit Preview		Dec 12, 2024 10:37pm
y-sweet-gendocs	⬜️ Ignored (Inspect)	Visit Preview		Dec 12, 2024 10:37pm

ellipsis-dev

❌ Changes requested. Reviewed everything up to fc6f466 in 45 seconds

More details

Looked at 163 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 2 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:404

Draft comment:
Remove or replace console.log with a proper logging mechanism for production use.
Reason this comment was not posted:
Confidence changes required: 50%
The console.log statement in the emit function is likely for debugging purposes and should be removed or replaced with a proper logging mechanism before production.

2. js-pkg/client/src/provider.ts:415

Draft comment:
Good practice to emit the sync event when the initial sync state changes to notify listeners.
Reason this comment was not posted:
Confidence changes required: 0%
The setSynced method is used to emit the sync event when the initial sync state changes. This is a good practice to notify listeners about the sync state.

Workflow ID: wflow_8NiC1LdksYthGKYd

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

js-pkg/client/src/provider.ts

ellipsis-dev

👍 Looks good to me! Incremental review on 2e61e9f in 19 seconds

More details

Looked at 25 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:412

Draft comment:
The setSynced method is redundant since this.synced is now public and can be updated directly. Consider removing this method and updating this.synced directly where needed.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_7SPsxs2ao427rOT6

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 3b9ff80 in 60 minutes and 50 seconds

More details

Looked at 12 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:409

Draft comment:
Good cleanup removing debug logging.
Reason this comment was not posted:
Confidence changes required: 0%
The removal of the console.log is good - it was just debug logging that could be noisy. The PR is implementing sync status events which is a useful feature. The implementation looks solid with proper message handling and state tracking. No issues found.

Workflow ID: wflow_parflct7f7DdkDXn

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 2bc9ec2 in 33 minutes and 16 seconds

More details

Looked at 13 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:50

Draft comment:
Good style improvement - adding space after new improves readability and is consistent with TypeScript style guides.
Reason this comment was not posted:
Confidence changes required: 0%
Looking at the WebSocketPolyfillType type definition, I notice that the spacing in the constructor signature was changed from new(url: string | URL) to new (url: string | URL). This is a style change that improves readability by adding a space after new. The change is good and consistent with TypeScript style guides.

Workflow ID: wflow_VOXYvC3nM5EVDUfk

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 2bc9ec2 in 39 minutes and 22 seconds

More details

Looked at 13 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:50

Draft comment:
Consider handling potential integer overflow for lastSyncSent and lastSyncAcked. Could reset both to 0 when they reach a large value, since only equality matters.
Reason this comment was not posted:
Confidence changes required: 50%
The change in the PR is a minor formatting change. However, the codebase has a potential issue with integer overflow for lastSyncSent and lastSyncAcked. This should be addressed to ensure long-term stability.

Workflow ID: wflow_Aqf5ArtJkDaeP8HN

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 5fd16c0 in 66 minutes and 41 seconds

More details

Looked at 14 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:110

Draft comment:
The sync status implementation looks good - removing initialSync and keeping synced aligns well with the more accurate sync status tracking described in the PR.
Reason this comment was not posted:
Confidence changes required: 0%
Looking at the changes in the PR, I notice that the initialSync field was removed but synced is kept. This is good as it aligns with the PR description about making the sync status more accurate. The changes look correct and implement the sync status tracking as described in the PR description using lastSyncSent and lastSyncAcked. The implementation appears sound and matches the intended design.

Workflow ID: wflow_vWY9rGnhdZK14iRF

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 670edd3 in 65 minutes and 31 seconds

More details

Looked at 15 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:427

Draft comment:
The removed auto-reconnect logic here could lead to missed reconnections in some edge cases. Consider adding back:

  private setStatus(status: YSweetStatus) {
    if (status.status === STATUS_DISCONNECTED && this.shouldConnect && !this.isConnecting) {
      this.connect()
    }

    if (this.status.status !== status.status) {
      this.status = status 
      this.emit('status', status)
    }
  }

Reason this comment was not posted:
Comment looked like it was already resolved.

Workflow ID: wflow_glBHMMjz19aAFhAl

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 37fbd54 in 36 seconds

More details

Looked at 77 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 3 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:430

Draft comment:
The setStatus method emits an object with a status property, but YSweetStatus is now a union of string literals. Consider updating the emitted value to match the new type definition.

      this.emit('status', status)

Reason this comment was not posted:
Comment looked like it was already resolved.

2. js-pkg/client/src/provider.ts:233

Draft comment:
The statusListener function should be updated to handle YSweetStatus as a union of string literals, not an object.

        let statusListener = (event: string) => {

Reason this comment was not posted:
Comment was on unchanged code.

3. js-pkg/client/src/provider.ts:298

Draft comment:
The setStatus method is called with an object, but YSweetStatus is now a union of string literals. Update the call to pass a string literal instead.

    this.setStatus(STATUS_CONNECTING)

Reason this comment was not posted:
Comment looked like it was already resolved.

Workflow ID: wflow_EcAYFYgUjAvv0uwk

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on a5981a9 in 46 seconds

More details

Looked at 717 lines of code in 7 files
Skipped 0 files when reviewing.
Skipped posting 7 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:399

Draft comment:
The connect() method is called recursively in websocketClose and websocketError, which can lead to an infinite loop if the connection keeps failing. Consider adding a check to ensure connect() is not called if it's already in the process of connecting.
Reason this comment was not posted:
Comment looked like it was already resolved.

2. js-pkg/react/src/main.tsx:98

Draft comment:
The variable isSynced is misleading. Consider renaming it to hasLocalChanges to better reflect its purpose.
Reason this comment was not posted:
Confidence changes required: 50%
The useHasLocalChanges hook in react/src/main.tsx uses useState to track isSynced, but the variable name is misleading. It should be hasLocalChanges to reflect its actual purpose.

3. js-pkg/react/src/main.tsx:100

Draft comment:
The variable isSynced is misleading. Consider renaming it to hasLocalChanges to better reflect its purpose.
Reason this comment was not posted:
Confidence changes required: 50%
The useHasLocalChanges hook in react/src/main.tsx uses useState to track isSynced, but the variable name is misleading. It should be hasLocalChanges to reflect its actual purpose.

4. js-pkg/react/src/main.tsx:102

Draft comment:
The variable isSynced is misleading. Consider renaming it to hasLocalChanges to better reflect its purpose.
Reason this comment was not posted:
Confidence changes required: 50%
The useHasLocalChanges hook in react/src/main.tsx uses useState to track isSynced, but the variable name is misleading. It should be hasLocalChanges to reflect its actual purpose.

5. js-pkg/react/src/main.tsx:120

Draft comment:
Ensure that the initial state of status is correctly set from the provider.
Reason this comment was not posted:
Confidence changes required: 30%
The useConnectionStatus hook in react/src/main.tsx uses useState to track status, which is fine, but ensure that the initial state is correctly set from the provider.

6. js-pkg/react/src/main.tsx:122

Draft comment:
Ensure that the initial state of status is correctly set from the provider.
Reason this comment was not posted:
Confidence changes required: 30%
The useConnectionStatus hook in react/src/main.tsx uses useState to track status, which is fine, but ensure that the initial state is correctly set from the provider.

7. js-pkg/react/src/main.tsx:124

Draft comment:
Ensure that the initial state of status is correctly set from the provider.
Reason this comment was not posted:
Confidence changes required: 30%
The useConnectionStatus hook in react/src/main.tsx uses useState to track status, which is fine, but ensure that the initial state is correctly set from the provider.

Workflow ID: wflow_QfDHCg6Xjq4uz7zJ

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 33b6704 in 9 seconds

More details

Looked at 159 lines of code in 4 files
Skipped 0 files when reviewing.
Skipped posting 2 drafted comments based on config settings.

1. js-pkg/client/src/ws-status.ts:78

Draft comment:
The change from EVENT_CONNECTION_STATUS to EVENT_STATUS in emit is intentional to unify event naming and aligns with the PR description's intent to handle sync status events.
Reason this comment was not posted:
Confidence changes required: 0%
The change from EVENT_CONNECTION_STATUS to EVENT_STATUS in emit is intentional to unify event naming and aligns with the PR description's intent to handle sync status events.

2. tests/src/index.test.ts:137

Draft comment:
The change from await createYjsProvider to createYjsProvider is intentional to remove unnecessary await since createYjsProvider is not an async function. This change is consistent across multiple test cases.
Reason this comment was not posted:
Confidence changes required: 0%
The change from await createYjsProvider to createYjsProvider in tests is intentional to remove unnecessary await since createYjsProvider is not an async function. This change is consistent across multiple test cases.

Workflow ID: wflow_FCJZFKMXGGn2X3ec

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

This is extracted from #353 and includes only the server-side change. It adds a custom message type that simply echoes back the content it received. Since the server processes messages sequentially, this is a way for a client to confirm that a message has been processed: - send some message - send a sync status message with a nonce - when you have received a message with the same nonce back, you can assume the message has been processed

ellipsis-dev

👍 Looks good to me! Incremental review on 55b2d18 in 15 seconds

More details

Looked at 92 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 3 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:150

Draft comment:
Consider logging a warning or error if the websocket is not open when attempting to send a message. This will help in debugging connection issues.
Reason this comment was not posted:
Confidence changes required: 50%
The send method is a good abstraction for sending messages over the websocket, but it should handle the case where the websocket is not open more gracefully.

2. js-pkg/client/src/provider.ts:174

Draft comment:
Consider adding a warning log if the websocket is not connected when trying to send an update. This will help in identifying issues when updates are not being sent due to connection problems.
Reason this comment was not posted:
Confidence changes required: 50%
The update method should log a warning if the websocket is not connected, similar to the previous implementation.

3. js-pkg/client/src/provider.ts:320

Draft comment:
Use the send method instead of directly accessing this.websocket?.send for consistency and to ensure the websocket is open before sending.
Reason this comment was not posted:
Confidence changes required: 50%
The receiveSyncMessage method should use the send method for consistency and to ensure the websocket is open before sending.

Workflow ID: wflow_kJnrUYrFE63QYoBX

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

…atus

paulgb · 2024-12-12T22:22:29Z

crates/y-sweet/src/server.rs

@@ -17,7 +17,6 @@ use futures::{SinkExt, StreamExt};
 use serde::Deserialize;
 use serde_json::{json, Value};
 use std::{
-    net::SocketAddr,


unrelated change that snuck in, but it's just removing an unnecessary import

ellipsis-dev

👍 Looks good to me! Incremental review on 42bd0f6 in 19 seconds

More details

Looked at 66 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:401

Draft comment:
Consider adding a delay before calling this.connect() to prevent rapid reconnect attempts in case of persistent connection failures.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_U9MyA2D50RXSNiFf

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

jakelazaroff

Only a couple minor comments but LGTM

jakelazaroff · 2024-12-12T22:22:27Z

js-pkg/client/src/provider.ts

+      this.on(EVENT_CONNECTION_STATUS, statusListener)
+    })
+
+    this.setupWs(clientToken)


Would you be opposed to inlining this? All the small methods that are only used once make the connection flow a bit difficult for me to follow.

jakelazaroff · 2024-12-12T22:29:41Z

js-pkg/client/src/provider.ts

  }

-  protected emit(eventName: YSweetEvent, data: any = null): void {
+  public emit(eventName: YSweetEvent | YWebsocketEvent, data: any = null): void {


Why make this public?

It's needed so that WebSocketCompatLayer can emit its own events, unfortunately.

ellipsis-dev

👍 Looks good to me! Incremental review on a9f4610 in 6 seconds

More details

Looked at 41 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. tests/src/index.test.ts:11

Draft comment:
The function waitForLocalChangesSync has been removed from util.ts. Ensure that no other parts of the codebase rely on this function, as its removal could lead to issues if still referenced elsewhere.
Reason this comment was not posted:
Confidence changes required: 50%
The removal of waitForLocalChangesSync seems intentional, but it's important to ensure that no other parts of the codebase rely on it.

Workflow ID: wflow_qeZddOZMwpEAJcmm

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 963f722 in 26 seconds

More details

Looked at 53 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 2 drafted comments based on config settings.

1. js-pkg/client/src/provider.ts:304

Draft comment:
The generateUrl function is defined twice in this file. Consider removing one of the definitions to avoid redundancy.
Reason this comment was not posted:
Comment was on unchanged code.

2. js-pkg/client/src/provider.ts:301

Draft comment:
The generateUrl function is defined twice in this file. Consider removing one of the definitions to avoid redundancy.
Reason this comment was not posted:
Marked as duplicate.

Workflow ID: wflow_3rvtxaMP6gmm5TP5

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

paulgb requested review from jakelazaroff and rolyatmax December 11, 2024 02:54

ellipsis-dev bot reviewed Dec 11, 2024

View reviewed changes

js-pkg/client/src/provider.ts Show resolved Hide resolved

ellipsis-dev bot reviewed Dec 11, 2024

View reviewed changes

vercel bot deployed to Preview – y-sweet-demos December 11, 2024 10:21 View deployment

Base automatically changed from paul/dis-3018-rewrite-ysweetprovider to main December 11, 2024 23:25

paulgb added 6 commits December 11, 2024 18:36

Rewrite provider

3ffc030

remove eventlistener which doesn't work on some nodes

62af956

improve reconnect logic

612b76d

improve reconnect logic

29bff55

Events for sync status

c388f7b

synced field works as in y-websocket

5fd16c0

paulgb force-pushed the paul/dis-2740-y-sweet-flag-for-connection-status branch from 2e61e9f to 5fd16c0 Compare December 11, 2024 23:45

paulgb added 4 commits December 11, 2024 18:46

remove console log

3b9ff80

rm unnecessary check

670edd3

fix accidental revert

f60ec34

format

2bc9ec2

vercel bot deployed to Preview – y-sweet-demos December 11, 2024 23:51 View deployment

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

simplify internal status representation

37fbd54

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

vercel bot deployed to Preview – y-sweet-demos December 12, 2024 15:54 View deployment

many improvements and demo

a5981a9

vercel bot deployed to Preview – y-sweet-gendocs December 12, 2024 20:56 View deployment

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

vercel bot deployed to Preview – y-sweet-debugger December 12, 2024 20:56 View deployment

vercel bot deployed to Preview – y-sweet-demos December 12, 2024 20:58 View deployment

fix tests

33b6704

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

vercel bot deployed to Preview – y-sweet-demos December 12, 2024 21:25 View deployment

paulgb mentioned this pull request Dec 12, 2024

Add echo endpoint to server for sync check #356

Merged

simplify websocket checks

55b2d18

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

Merge branch 'main' into paul/dis-2740-y-sweet-flag-for-connection-st…

d7acf2b

…atus

paulgb commented Dec 12, 2024

View reviewed changes

deprecate y-websocket compat functions

42bd0f6

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

jakelazaroff approved these changes Dec 12, 2024

View reviewed changes

vercel bot deployed to Preview – y-sweet-demos December 12, 2024 22:33 View deployment

rm unused helper

a9f4610

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

inline setupWs

963f722

ellipsis-dev bot reviewed Dec 12, 2024

View reviewed changes

vercel bot deployed to Preview – y-sweet-demos December 12, 2024 22:37 View deployment

paulgb merged commit 262d02c into main Dec 12, 2024
11 checks passed

paulgb deleted the paul/dis-2740-y-sweet-flag-for-connection-status branch December 12, 2024 22:40

This was referenced Dec 12, 2024

State hooks #306

Closed

Project Roadmap #311

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement events for sync status #353

Implement events for sync status #353

paulgb commented Dec 11, 2024 •

edited

Loading

vercel bot commented Dec 11, 2024 •

edited

Loading

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

paulgb Dec 12, 2024

ellipsis-dev bot left a comment

jakelazaroff left a comment

jakelazaroff Dec 12, 2024

jakelazaroff Dec 12, 2024

paulgb Dec 12, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

Implement events for sync status #353

Implement events for sync status #353

Conversation

paulgb commented Dec 11, 2024 • edited Loading

vercel bot commented Dec 11, 2024 • edited Loading

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

paulgb Dec 12, 2024

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

jakelazaroff left a comment

Choose a reason for hiding this comment

jakelazaroff Dec 12, 2024

Choose a reason for hiding this comment

jakelazaroff Dec 12, 2024

Choose a reason for hiding this comment

paulgb Dec 12, 2024

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

paulgb commented Dec 11, 2024 •

edited

Loading

vercel bot commented Dec 11, 2024 •

edited

Loading