-
Notifications
You must be signed in to change notification settings - Fork 3.2k
improvement(avatar): use selection-update as the source of truth for presence, ignore other socket ops #2866
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
…presence, ignore other socket ops
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryChanged Key changes:
Rationale: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Client Socket
participant Provider as SocketProvider
participant Server as Socket Server
participant Room as RoomManager
Note over Client,Room: User Joins Workflow
Client->>Server: join-workflow(workflowId)
Server->>Room: createUserPresence(userId, userName, avatarUrl)
Room->>Room: room.users.set(socketId, userPresence)
Server->>Room: broadcastPresenceUpdate(workflowId)
Room->>Client: presence-update([...allUsers])
Client->>Provider: setPresenceUsers(users)
Note over Provider: User presence list initialized
Note over Client,Room: Cursor Movement (After PR)
Client->>Server: cursor-update({x, y})
Server->>Client: cursor-update(data)
Client->>Provider: Check if user exists
alt User exists in presenceUsers
Provider->>Provider: Update cursor for existing user
else User NOT in presenceUsers
Provider->>Provider: Return early, ignore update
end
Note over Client,Room: Selection Change (After PR)
Client->>Server: selection-update({type, id})
Server->>Client: selection-update(data)
Client->>Provider: Check if user exists
alt User exists in presenceUsers
Provider->>Provider: Update selection for existing user
else User NOT in presenceUsers
Provider->>Provider: Return early, ignore update
end
|
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.
1 file reviewed, 2 comments
| setPresenceUsers((prev) => { | ||
| const existingIndex = prev.findIndex((user) => user.socketId === data.socketId) | ||
| if (existingIndex !== -1) { | ||
| return prev.map((user) => | ||
| user.socketId === data.socketId ? { ...user, cursor: data.cursor } : user | ||
| ) | ||
| if (existingIndex === -1) { | ||
| return prev | ||
| } |
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.
style: If a cursor-update arrives before presence-update (timing issue during connection), cursor position will be silently dropped. Consider logging when this occurs for debugging.
| setPresenceUsers((prev) => { | |
| const existingIndex = prev.findIndex((user) => user.socketId === data.socketId) | |
| if (existingIndex !== -1) { | |
| return prev.map((user) => | |
| user.socketId === data.socketId ? { ...user, cursor: data.cursor } : user | |
| ) | |
| if (existingIndex === -1) { | |
| return prev | |
| } | |
| setPresenceUsers((prev) => { | |
| const existingIndex = prev.findIndex((user) => user.socketId === data.socketId) | |
| if (existingIndex === -1) { | |
| logger.debug('Received cursor-update for unknown user', { socketId: data.socketId }) | |
| return prev | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/providers/socket-provider.tsx
Line: 407:411
Comment:
**style:** If a `cursor-update` arrives before `presence-update` (timing issue during connection), cursor position will be silently dropped. Consider logging when this occurs for debugging.
```suggestion
setPresenceUsers((prev) => {
const existingIndex = prev.findIndex((user) => user.socketId === data.socketId)
if (existingIndex === -1) {
logger.debug('Received cursor-update for unknown user', { socketId: data.socketId })
return prev
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| setPresenceUsers((prev) => { | ||
| const existingIndex = prev.findIndex((user) => user.socketId === data.socketId) | ||
| if (existingIndex !== -1) { | ||
| return prev.map((user) => | ||
| user.socketId === data.socketId ? { ...user, selection: data.selection } : user | ||
| ) | ||
| if (existingIndex === -1) { | ||
| return prev | ||
| } |
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.
style: Same as the cursor-update handler - consider adding debug logging for dropped selection updates.
| setPresenceUsers((prev) => { | |
| const existingIndex = prev.findIndex((user) => user.socketId === data.socketId) | |
| if (existingIndex !== -1) { | |
| return prev.map((user) => | |
| user.socketId === data.socketId ? { ...user, selection: data.selection } : user | |
| ) | |
| if (existingIndex === -1) { | |
| return prev | |
| } | |
| setPresenceUsers((prev) => { | |
| const existingIndex = prev.findIndex((user) => user.socketId === data.socketId) | |
| if (existingIndex === -1) { | |
| logger.debug('Received selection-update for unknown user', { socketId: data.socketId }) | |
| return prev | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/providers/socket-provider.tsx
Line: 420:424
Comment:
**style:** Same as the cursor-update handler - consider adding debug logging for dropped selection updates.
```suggestion
setPresenceUsers((prev) => {
const existingIndex = prev.findIndex((user) => user.socketId === data.socketId)
if (existingIndex === -1) {
logger.debug('Received selection-update for unknown user', { socketId: data.socketId })
return prev
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Summary
Type of Change
Testing
Tested manually
Checklist