Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements WebSocket support for real-time collaboration in the document editor, enabling bidirectional communication for sending and receiving editing steps. The changes add WebSocket-based step synchronization with HTTP fallback, error handling for conflicts and failures, and connection state management.
Changes:
- Added WebSocket connection management with send/receive capabilities and automatic reconnection
- Implemented error handling for WebSocket conflicts and failures with retry logic
- Added code formatting improvements (line breaks, semicolons) for better readability
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/api/types.ts | Defines new StepsErrorListener type for WebSocket error callbacks |
| src/api/Api.ts | Implements WebSocket connection with send/receive methods, error handling, and connection state tracking |
| src/api/StepsExchanger.ts | Integrates WebSocket for sending/receiving steps with HTTP fallback and conflict resolution |
| src/components/comments/CommentActions.tsx | Adds semicolon before type assertion to prevent ASI issues |
| src/components/comments/ReplyBox.tsx | Reformats PrimaryButton props across multiple lines |
| src/components/track-changes/suggestion-list/SuggestionList.tsx | Reformats arrow function across multiple lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return new Promise((resolve) => { | ||
| pendingStepsResolve = resolve | ||
|
|
||
| const timeout = setTimeout(() => { | ||
| if (pendingStepsResolve === resolve) { | ||
| pendingStepsResolve = null | ||
| resolve(undefined) | ||
| } | ||
| }, 10000) | ||
|
|
||
| this.getAuthToken() | ||
| .then((token) => { | ||
| return ws.send( | ||
| JSON.stringify({ | ||
| type: 'getSteps', | ||
| version, | ||
| token, | ||
| }) | ||
| ) | ||
| }) | ||
| .catch(() => { | ||
| clearTimeout(timeout) | ||
| pendingStepsResolve = null | ||
| resolve(undefined) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The timeout is not cleared when the promise resolves successfully. If a 'stepsResponse' arrives and resolves the promise, the timeout callback will still fire after 10 seconds, potentially causing issues. Add clearTimeout(timeout) in the onMessage handler when resolving pendingStepsResolve.
| private handleWebSocketError(error: 'conflict' | 'failed') { | ||
| if (error === 'conflict') { | ||
| if (this.attempt < MAX_ATTEMPTS) { | ||
| this.newStepsListener() | ||
| this.attempt++ | ||
| } else { | ||
| this.saveStatus.setValue('failed') | ||
| this.attempt = 0 | ||
| } | ||
| } else { | ||
| this.saveStatus.setValue('failed') | ||
| this.attempt = 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
When a conflict error occurs and retries are attempted, this.newStepsListener() is called without arguments. However, looking at the class structure, this appears to be a listener that should receive steps data, not trigger a retry. The retry logic should likely call a method to refetch steps or resend pending changes instead.
| return ws.send( | ||
| JSON.stringify({ | ||
| type: 'getSteps', | ||
| version, | ||
| token, | ||
| }) | ||
| ) |
There was a problem hiding this comment.
The WebSocket send operation is not checked for success. ws.send() returns void but can throw an error if the connection is closed. This could cause unhandled exceptions. Wrap the send call in a try-catch block.
| return ws.send( | |
| JSON.stringify({ | |
| type: 'getSteps', | |
| version, | |
| token, | |
| }) | |
| ) | |
| try { | |
| ws.send( | |
| JSON.stringify({ | |
| type: 'getSteps', | |
| version, | |
| token, | |
| }) | |
| ) | |
| } catch { | |
| clearTimeout(timeout) | |
| pendingStepsResolve = null | |
| resolve(undefined) | |
| } |
No description provided.