Skip to content

fix(sockets): joining currently deleted workflow#4004

Merged
icecrasher321 merged 2 commits intostagingfrom
fix/client-join-error
Apr 7, 2026
Merged

fix(sockets): joining currently deleted workflow#4004
icecrasher321 merged 2 commits intostagingfrom
fix/client-join-error

Conversation

@icecrasher321
Copy link
Copy Markdown
Collaborator

Summary

Joining current workflow when deleted.

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Apr 7, 2026 2:29am

Request Review

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 7, 2026

PR Summary

Medium Risk
Touches socket room join/rejoin logic during deletion and session-expiry flows; a small conditional bug here could prevent legitimate reconnects or leave clients in the wrong room.

Overview
Prevents the client from automatically join-workflowing a workflow that was just deleted while the URL/router state is still catching up.

Adds a deletedWorkflowIdRef guard set on workflow-deleted, used to block session-expiry rejoin attempts (operation-forbidden) and to short-circuit the URL-driven room switch effect until the workflow ID changes (then clears the guard).

Reviewed by Cursor Bugbot for commit d31bd1b. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 7, 2026

Greptile Summary

This PR fixes a race condition in SocketProvider where a workflow-deleted socket event clears currentWorkflowId, but before router.push() completes navigating away, the join-workflow useEffect re-fires — still seeing the deleted workflow ID in the URL — and emits a join-workflow request for a workflow that no longer exists. A secondary code path via operation-forbidden / SESSION_ERROR could trigger the same spurious rejoin.

The fix introduces deletedWorkflowIdRef (useRef<string | null>) to track the most recently deleted workflow ID, and both the join-workflow useEffect and the SESSION_ERROR rejoin path check this ref to bail out while the URL still points to the deleted workflow. The ref is correctly cleared when navigating to a new workflow or when urlWorkflowId drops to undefined.

Key changes:

  • New deletedWorkflowIdRef ref initialized to null
  • Set in the workflow-deleted socket event handler when the current workflow is deleted
  • Checked in the join-workflow useEffect to block a rejoin when the URL still references the deleted workflow
  • Checked in the operation-forbidden handler to prevent SESSION_ERROR-triggered rejoins for deleted workflows
  • Cleared when navigating to a new workflow or when the URL workflow param becomes undefined

One style finding (P2): deletedWorkflowIdRef.current is mutated inside a React state updater function, which should be pure. It is idempotent and safe in practice, but should be moved outside the updater for idiomatic React code.

Confidence Score: 5/5

Safe to merge — the fix correctly addresses the race condition with no correctness issues

All findings are P2 style suggestions. The bug fix is logically sound and the deletedWorkflowIdRef approach correctly handles all identified code paths (join-workflow effect, SESSION_ERROR rejoin, and cleanup on navigation). The single flagged item (side effect inside a state updater) is idempotent in practice and follows the same pre-existing pattern already present in the file.

No files require special attention

Important Files Changed

Filename Overview
apps/sim/app/workspace/providers/socket-provider.tsx Adds deletedWorkflowIdRef to prevent the join-workflow effect from re-joining a just-deleted workflow during the router.push() propagation delay

Sequence Diagram

sequenceDiagram
    actor User
    participant Server
    participant SocketProvider
    participant Router

    User->>Server: delete workflow
    Server->>SocketProvider: emit("workflow-deleted", { workflowId })
    SocketProvider->>SocketProvider: setCurrentWorkflowId(null)
    SocketProvider->>SocketProvider: deletedWorkflowIdRef.current = workflowId
    SocketProvider->>Router: workflowDeleted handler fires → router.push(newUrl)

    Note over SocketProvider: React re-renders before router.push() settles
    SocketProvider->>SocketProvider: join-workflow useEffect fires
    SocketProvider->>SocketProvider: urlWorkflowId still == deleted ID
    SocketProvider->>SocketProvider: deletedWorkflowIdRef === urlWorkflowId → return early ✓

    Router->>SocketProvider: urlWorkflowId changes (navigation complete)
    SocketProvider->>SocketProvider: deletedWorkflowIdRef.current = null (cleanup)
    SocketProvider->>Server: emit("join-workflow", { workflowId: newId })

    alt operation-forbidden with SESSION_ERROR
        Server->>SocketProvider: emit("operation-forbidden", { type: "SESSION_ERROR" })
        SocketProvider->>SocketProvider: deletedWorkflowIdRef === urlWorkflowId → skip rejoin ✓
    end
Loading

Comments Outside Diff (1)

  1. apps/sim/app/workspace/providers/socket-provider.tsx, line 375-382 (link)

    P2 Side effect inside React state updater

    React requires state updater functions to be pure — no side effects. In React 18 Strict Mode, updaters are intentionally called twice in development to surface impurity. Both the new deletedWorkflowIdRef.current assignment and the pre-existing setPresenceUsers([]) call are idempotent in practice, so no production bug will occur, but the pattern is non-idiomatic and goes against the React docs guidance.

    Consider moving both side effects outside the updater:

    This is safe because the workflow-deleted event is emitted by the server for the specific workflow — the side effects should always run when this event arrives, regardless of whether the updater actually changes state.

Reviews (1): Last reviewed commit: "fix(sockets): joining currently deleted ..." | Re-trigger Greptile

@icecrasher321
Copy link
Copy Markdown
Collaborator Author

bugbot run

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit d31bd1b. Configure here.

@icecrasher321 icecrasher321 merged commit 609ba61 into staging Apr 7, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant