fix(ui): Improve project import synchronization and error handling #995
Conversation
WalkthroughThe changes involve restructuring the asynchronous handling within the project import hook. The update to the Y.Doc instance is now executed within a transaction triggered by the successful synchronization of the WebSocket connection. The redundant asynchronous wrapper for setting up the WebSocket provider has been removed, and the error message has been clarified. Additionally, the order of dependencies in the Changes
Sequence Diagram(s)sequenceDiagram
participant IH as ImportHandler
participant WP as WebSocketProvider
participant YD as Y.Doc
IH->>WP: Initialize connection
WP->>IH: Emit "sync" event
IH->>YD: Begin transaction and apply update
YD-->>IH: Acknowledge update
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Nitpick comments (2)
ui/src/hooks/useProjectImport.ts (2)
80-94: Good use of Promise to handle sync event.The introduction of a Promise to manage the WebSocket synchronization event is a good approach for handling the asynchronous nature of the sync process. However, consider adding a timeout mechanism to prevent potential deadlocks if the sync event never fires.
- await new Promise<void>((resolve) => { + await Promise.race([ + new Promise<void>((resolve) => { + yWebSocketProvider.once("sync", () => { + yDoc.transact(() => { + Y.applyUpdate(yDoc, yDocBinary); + }); + + const yWorkflows = yDoc.getArray<YWorkflow>("workflows"); + if (!yWorkflows.length) { + console.warn("Imported project has no workflows"); + } + + setIsProjectImporting(false); + resolve(); + }); + }), + new Promise<void>((_, reject) => { + setTimeout(() => reject(new Error("Sync timeout")), 30000); // 30 seconds timeout + }), + ]);
96-99: Enhance error handling with user feedback.The error handling only logs to the console and resets the loading state. Consider providing user feedback about the error.
try { // ... } catch (error) { console.error("Failed to import project:", error); + // Add user feedback here, e.g., notification + // if you have a notification system, you might use it like: + // notifyError(t("Failed to import project"), error); setIsProjectImporting(false); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/hooks/useProjectImport.ts(1 hunks)
🔇 Additional comments (3)
ui/src/hooks/useProjectImport.ts (3)
82-84: Excellent use of transaction for atomic updates.Wrapping the Y.applyUpdate call in a transaction is a good practice. YJS transactions ensure that all updates within the transaction are applied atomically, which prevents inconsistent states and improves performance by batching updates.
101-101: Dependency array ordering change is correct.The reordering of dependencies in the useCallback hook doesn't affect functionality as long as all dependencies are included.
108-108: Good addition of fileInputRef to return object.Adding fileInputRef to the return object makes sense if there's a need to access this ref from outside the hook, allowing more flexible usage by consumers.
ui/src/hooks/useProjectImport.ts
Outdated
| const yDoc = new Y.Doc(); | ||
| Y.applyUpdate(yDoc, yDocBinary); | ||
|
|
||
| const { websocket } = config(); | ||
|
|
||
| if (websocket && projectMeta) { | ||
| (async () => { | ||
| const token = await getAccessToken(); | ||
|
|
||
| const yWebSocketProvider = new WebsocketProvider( | ||
| websocket, | ||
| `${project.id}:${DEFAULT_ENTRY_GRAPH_ID}`, | ||
| yDoc, | ||
| { params: { token } }, | ||
| ); | ||
|
|
||
| const token = await getAccessToken(); | ||
|
|
||
| const yWebSocketProvider = new WebsocketProvider( | ||
| websocket, | ||
| `${project.id}:${DEFAULT_ENTRY_GRAPH_ID}`, | ||
| yDoc, | ||
| { params: { token } }, | ||
| ); | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| yWebSocketProvider.once("sync", () => { | ||
| yDoc.transact(() => { | ||
| Y.applyUpdate(yDoc, yDocBinary); | ||
| }); | ||
|
|
||
| const yWorkflows = yDoc.getArray<YWorkflow>("workflows"); | ||
| if (!yWorkflows.length) { | ||
| console.warn("Imported project has no workflows"); | ||
| } | ||
|
|
||
| setIsProjectImporting(false); | ||
| resolve(); | ||
| }); | ||
| })(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing WebSocket provider cleanup.
The WebSocket provider is created but never cleaned up, which could lead to memory leaks if the component using this hook unmounts before the sync is complete.
Consider adding a cleanup function using a useEffect hook at the component level:
useEffect(() => {
let yWebSocketProvider: WebsocketProvider | undefined;
// Assign the provider to this variable when creating it
return () => {
// Clean up WebSocket provider if it exists
if (yWebSocketProvider) {
yWebSocketProvider.disconnect();
}
};
}, []);Alternatively, you could return a cleanup function from the hook itself that consumers can call when unmounting.
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit