59 yjs websocket refactor - #60
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
corates-workers-prod | 374ea9d | Dec 16 2025, 04:04 AM |
|
Caution Review failedThe pull request is closed. WalkthroughRefactors client WebSocket sync to use y-websocket's WebsocketProvider, updates the worker durable object to use binary y-protocols/awareness via lib0/y-protocols, adds related dependencies, and introduces an awareness integration plan and primitives for presence UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant Provider as WebsocketProvider
participant WS as WebSocket
participant Worker as ProjectDoc (Durable Object)
participant YDoc as Y.Doc
participant Awareness as Awareness
Client->>Provider: connect()
Provider->>WS: open WebSocket
WS-->>Provider: connection established
Provider->>Worker: send binary sync (y-protocol)
Worker->>YDoc: apply remote update
Worker->>Worker: broadcastBinary(sync) -> other sessions
Client->>Awareness: setLocalState (cursor/user)
Awareness->>Provider: awareness update
Provider->>WS: send binary awareness message
WS->>Worker: receive awareness binary
Worker->>Worker: update Awareness, broadcastBinary(awareness)
Note over Client,Provider: Provider emits status/sync events
Provider-->>Client: status(sync/connected/connecting)
Provider-->>Client: sync event -> onSync callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/web/package.json(1 hunks)packages/web/src/primitives/useProject/connection.js(1 hunks)packages/web/src/primitives/useProject/index.js(0 hunks)packages/workers/package.json(1 hunks)packages/workers/src/durable-objects/ProjectDoc.js(9 hunks)yjs-websocket-refactor.plan.md(1 hunks)
💤 Files with no reviewable changes (1)
- packages/web/src/primitives/useProject/index.js
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/package.jsonyjs-websocket-refactor.plan.mdpackages/web/package.jsonpackages/workers/src/durable-objects/ProjectDoc.jspackages/web/src/primitives/useProject/connection.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/durable-objects/ProjectDoc.jspackages/web/src/primitives/useProject/connection.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/durable-objects/ProjectDoc.jspackages/web/src/primitives/useProject/connection.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Zod for schema and input validation
Files:
packages/workers/src/durable-objects/ProjectDoc.jspackages/web/src/primitives/useProject/connection.js
packages/workers/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Better-Auth for authentication and user management
Files:
packages/workers/src/durable-objects/ProjectDoc.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx,js,ts}: When you need to compute a value based on props or state in SolidJS, usecreateMemoto ensure it updates reactively
For complex state or state objects in SolidJS, use Solid'screateStorefor better performance and reactivity
Create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Use Solid'screateStorefor complex state or state objects for better performance and reactivity
Create reusable logic in 'primitives' (hooks) that can be shared across components to keep components clean and focused on rendering
Files:
packages/web/src/primitives/useProject/connection.js
🧠 Learnings (1)
📚 Learning: 2025-12-16T03:19:31.899Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T03:19:31.899Z
Learning: Applies to packages/workers/**/*.{ts,tsx,js,jsx} : Use Better-Auth for authentication and user management
Applied to files:
packages/workers/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (11)
packages/workers/package.json (1)
27-30: Dependencies align with the y-websocket migration.The additions of
lib0andy-protocolsare the correct dependencies for implementing binary protocol sync and awareness on the backend. These are maintained by the Yjs project and are the standard libraries for this purpose.packages/workers/src/durable-objects/ProjectDoc.js (4)
1-10: Imports and protocol constants are correctly defined.The imports from
y-protocolsandlib0are appropriate, and the message type constants match the y-websocket protocol specification.
276-316: Document initialization with awareness and listeners is well-structured.The setup correctly:
- Creates an
Awarenessinstance tied to the Y.Doc- Persists the full document state on every update (ensuring durability across DO restarts)
- Broadcasts sync updates and awareness changes using the binary protocol
390-426: Binary message handling follows y-protocols conventions correctly.The switch-based handling of sync and awareness messages is well-implemented. The check for
encoding.length(encoder) > 1correctly determines if there's a response to send beyond the message type byte.
524-533: Binary broadcast helper is correctly implemented.The implementation properly checks
readyState === 1(WebSocket.OPEN) and excludes the origin connection from receiving its own updates.packages/web/package.json (1)
39-39: Frontend dependency for y-websocket provider is appropriate.The
y-websocketpackage provides theWebsocketProviderused inconnection.jsfor automatic reconnection, sync protocol handling, and awareness support.yjs-websocket-refactor.plan.md (1)
1-339: Plan document provides clear migration guidance.The phased approach is well-structured, covering backend protocol implementation, frontend provider migration, optional awareness features, and testing. The code examples align with the actual implementation in the PR.
packages/web/src/primitives/useProject/connection.js (4)
1-9: Migration to WebsocketProvider simplifies connection management.The refactor correctly adopts
y-websocket'sWebsocketProvider, delegating reconnection logic, binary protocol handling, and awareness to the library.
24-43: Provider initialization and event handling are correctly implemented.The
WebsocketProviderURL construction correctly produces the expected${wsUrl}/${projectId}pattern that matches the backend route. Status and sync event handlers appropriately update the connection state.
45-68: Error and membership rejection handling is well-designed.The handling of code 1008 (Policy Violation) for membership denial and setting
shouldConnect = falseto prevent infinite reconnection attempts is the correct approach.
71-108: Lifecycle and utility functions provide clean API.The
disconnect(),reconnect(), and accessor functions maintain the expected contract. The exposedgetAwareness()enables future awareness features as outlined in Phase 3 of the plan.
…tyBowman/corates into 59-yjs-websocket-refactor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/workers/src/durable-objects/ProjectDoc.js (1)
415-430: Consider using y-protocols Awareness API instead of manual decoding.The manual extraction of clientId from the binary awareness update (lines 421-429) assumes a specific binary format and only captures the first clientId. While this works for typical cases, it's fragile and could break if the awareness update structure varies.
A more robust approach would be to track clientIds through the awareness state changes or use the Awareness API directly:
case messageAwareness: { const awarenessUpdate = decoding.readVarUint8Array(decoder); + + // Decode awareness update to extract client IDs + const decodedUpdate = awarenessProtocol.decodeAwarenessUpdate(awarenessUpdate); + + // Store all client IDs from this connection (typically just one) + const session = this.sessions.get(server); + if (session) { + decodedUpdate.forEach(({ clientID }) => { + if (session.awarenessClientId === null) { + session.awarenessClientId = clientID; + } + }); + } + awarenessProtocol.applyAwarenessUpdate(this.awareness, awarenessUpdate, server); - - // Extract and store the client's awareness ID from the update - // The first client ID in the update is typically the sender's ID - const awarenessDecoder = decoding.createDecoder(awarenessUpdate); - const len = decoding.readVarUint(awarenessDecoder); - if (len > 0) { - const clientId = decoding.readVarUint(awarenessDecoder); - const session = this.sessions.get(server); - if (session && session.awarenessClientId === null) { - session.awarenessClientId = clientId; - } - } break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/workers/src/durable-objects/ProjectDoc.js(9 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/durable-objects/ProjectDoc.js
**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/durable-objects/ProjectDoc.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/durable-objects/ProjectDoc.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Zod for schema and input validation
Files:
packages/workers/src/durable-objects/ProjectDoc.js
packages/workers/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Better-Auth for authentication and user management
Files:
packages/workers/src/durable-objects/ProjectDoc.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (4)
packages/workers/src/durable-objects/ProjectDoc.js (4)
1-11: LGTM! Binary protocol setup is clean.The imports for y-protocols and lib0 are correctly structured, and the message type constants align with the y-websocket protocol specification.
28-36: LGTM! Constructor properly initializes session tracking.The change from Set to Map allows tracking per-connection metadata (user and awarenessClientId), which is necessary for the binary protocol implementation.
277-318: LGTM! Binary protocol and awareness integration are well-implemented.The initialization correctly:
- Creates an Awareness instance tied to the Y.Doc
- Persists full document state on every update
- Broadcasts binary-encoded sync and awareness messages to connected clients
- Excludes the origin from broadcasts to prevent echo
543-552: LGTM! Binary broadcast implementation is correct.The method properly:
- Excludes the origin connection to prevent echo
- Checks
readyState === 1(OPEN) before sending- Sends the binary message to all other connected clients
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
yjs-awareness.plan.md (4)
34-57: Initialize awareness state immediately in useAwareness hook.The
useAwarenessimplementation doesn't callupdateUsers()after setting up the listener. This means the reactiveuserssignal remains empty until the first awareness change event, introducing a potential delay or incomplete initial state.Apply this diff to populate the initial state:
export function useAwareness(awareness) { const [users, setUsers] = createSignal([]); if (!awareness) return { users }; const updateUsers = () => { const states = []; awareness.getStates().forEach((state, clientId) => { if (clientId !== awareness.clientID && state.user) { states.push({ clientId, ...state }); } }); setUsers(states); }; + updateUsers(); awareness.on('change', updateUsers); onCleanup(() => awareness.off('change', updateUsers)); return { users }; }
59-65: Add implementation details for PresenceIndicator and CollaboratorCursors components.The plan provides high-level descriptions but lacks code scaffolding or implementation guidance for these two components. Adding skeleton code or more detailed requirements would help with Phase 1 implementation clarity.
Consider adding code scaffolds similar to the
useAwarenessexample, such as:// PresenceIndicator.jsx import { useAwareness } from '../primitives/useAwareness'; export function PresenceIndicator({ awareness }) { const { users } = useAwareness(awareness); return ( <div class="presence-indicator"> {users().map(user => ( <Avatar key={user.clientId} user={user.user} /> ))} </div> ); }// CollaboratorCursors.jsx import { useAwareness } from '../primitives/useAwareness'; export function CollaboratorCursors({ awareness }) { const { users } = useAwareness(awareness); return ( <div class="cursor-overlay"> {users().map(user => user.cursor && ( <Cursor key={user.clientId} user={user.user} position={user.cursor} /> ) )} </div> ); }
13-22: Clarify integration point for setting awareness state in useProject.The example shows calling
awareness?.setLocalState()in useProject, but it doesn't specify when or where within the hook this should occur (e.g., on mount, after user context is available). Consider adding a note about initialization timing.Add clarification to the integration example:
// In useProject.js or wherever connection is consumed const awareness = connectionManager.getAwareness(); + // Set initial local user state after user context is loaded awareness?.setLocalState({ user: { id: user.id, name: user.name, color: getUserColor(user.id) }, cursor: null, // Will be updated by editor });
69-75: Add acceptance criteria for Phase 2 testing.The Phase 2 section includes a checkbox for awareness updates and multi-client visibility, but lacks acceptance criteria or expected outcomes. Consider defining what "propagate" and "see each other's presence" mean in testable terms.
Expand the Phase 2 checklist with concrete acceptance criteria:
## Phase 2: Testing & Migration ### 2.1 Test locally - [ ] Awareness updates propagate + - Verify: Set user state in one client, confirm visible in others within 500ms - [ ] Multiple clients see each other's presence + - Verify: Open 2+ tabs, confirm all connected users appear in PresenceIndicator + - Verify: Cursor updates sync across clients when moving in editorpackages/workers/src/durable-objects/ProjectDoc.js (2)
546-551: Consider using WebSocket.OPEN constant for readability.The broadcast logic is correct, but using the magic number
1for the ready state is less readable thanWebSocket.OPEN.Apply this diff to improve readability:
broadcastBinary(message, exclude = null) { this.sessions.forEach((sessionData, ws) => { - if (ws !== exclude && ws.readyState === 1) { + if (ws !== exclude && ws.readyState === WebSocket.OPEN) { ws.send(message); } }); }
419-429: Extract client ID from Awareness instance instead of parsing binary.The manual binary parsing to extract the client ID is fragile and tightly coupled to the lib0 decoding format. After calling
awarenessProtocol.applyAwarenessUpdate(), the Awareness instance automatically tracks client IDs. You can:
- Listen to the awareness 'update' or 'change' event, which emits the
addedclient ID array- Call
this.awareness.getStates()to retrieve the Map of all clients keyed by clientIDEither approach eliminates manual binary parsing while being more maintainable and resilient to format changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/workers/src/durable-objects/ProjectDoc.js(9 hunks)yjs-awareness.plan.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/durable-objects/ProjectDoc.js
**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/workers/src/durable-objects/ProjectDoc.jsyjs-awareness.plan.md
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Files:
packages/workers/src/durable-objects/ProjectDoc.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Zod for schema and input validation
Files:
packages/workers/src/durable-objects/ProjectDoc.js
packages/workers/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Better-Auth for authentication and user management
Files:
packages/workers/src/durable-objects/ProjectDoc.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: corates
- GitHub Check: Workers Builds: corates-workers-prod
🔇 Additional comments (13)
packages/workers/src/durable-objects/ProjectDoc.js (13)
2-5: LGTM! Binary protocol imports are correctly structured.The imports for
y-protocolsandlib0are properly configured to support the binary WebSocket protocol refactoring.
8-10: LGTM! Message type constants match y-websocket protocol.The constants correctly define the binary message types for sync and awareness as per the y-websocket specification.
32-35: LGTM! Session tracking structure properly updated.The refactored session tracking now stores both user information and awareness client IDs, which enables proper cleanup when connections close.
145-145: LGTM! Centralized broadcast approach improves consistency.The refactoring correctly delegates update broadcasting to the Y.doc update listener, eliminating duplicate broadcast logic.
202-202: LGTM! Consistent with centralized broadcast pattern.
280-280: LGTM! Awareness properly initialized with Y.Doc.The awareness instance is correctly tied to the Y.Doc, enabling collaborative presence tracking.
367-367: LGTM! Session tracking properly stores awarenessClientId.The session structure now correctly stores both user info and awareness client ID for proper cleanup on disconnect.
373-375: LGTM! Delegating sync initiation to the client.Correctly letting the y-websocket client initiate the sync handshake, which follows the standard y-websocket protocol flow.
380-389: LGTM! Binary data handling is robust.The code correctly handles both ArrayBuffer and Blob message formats and appropriately warns about unexpected string messages.
440-451: LGTM! The optional chaining issue has been fixed.The condition now correctly uses
session && session.awarenessClientId != null(loose equality), which properly guards against bothnullandundefinedvalues. This fixes the critical issue identified in the previous review wheresession?.awarenessClientId !== nullwould letundefinedpass through.Based on previous review comments, the fix was correctly applied.
303-316: The awareness encoding is correct and aligns with y-protocols specification. The implementation properly wraps theencodeAwarenessUpdateresult (which handles spec-compliant encoding of client states) with a message type identifier and length prefix. No changes needed.
397-413: Sync message handling is correctly implemented.The code properly follows y-protocols patterns:
syncProtocol.readSyncMessagehandles all sync steps (1, 2, and updates) internally and writes appropriate replies to the encoder- The condition
encoding.length(encoder) > 1correctly detects if a response exists, accounting for the 1-byte messageSync header already written- The
serverparameter is correctly passed as the provider for update attribution;serveris the WebSocket connection from the WebSocketPair
290-301: Binary encoding implementation correctly follows y-protocols specification.The encoding sequence is correct:
writeVarUint(encoder, messageSync)writes the protocol id (0), andsyncProtocol.writeUpdate(encoder, update)is the proper y-protocols API call that internally writes the sync message type (2) and the update payload as a varUint8Array. This matches the required format for sync protocol update messages.
59 yjs websocket refactor
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.