Skip to content

fix(socket): persist outbound edges from locked blocks#3404

Merged
waleedlatif1 merged 4 commits intostagingfrom
waleedlatif1/fix-locked-block-connections
Mar 3, 2026
Merged

fix(socket): persist outbound edges from locked blocks#3404
waleedlatif1 merged 4 commits intostagingfrom
waleedlatif1/fix-locked-block-connections

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • Server-side edge add operation was checking if source block is protected, preventing outbound connections from locked blocks from being persisted
  • Client-side already correctly only checks target block protection
  • Removed source block protection check to align server with client behavior

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

vercel bot commented Mar 3, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 3, 2026 8:45pm

Request Review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Greptile Summary

This PR fixes a server-side bug where outbound edges from locked blocks were being incorrectly blocked from persisting, by removing the source block protection check from all four edge operation paths in operations.ts — single add, single remove, batch add, and batch remove. The fix aligns the server with the already-correct client-side behavior defined in isEdgeProtected (block-protection-utils.ts), which only checks the target block's lock status.

  • Bug fix is correct: The client's isEdgeProtected explicitly documents that "outbound connections from locked blocks are allowed to be modified" — only inbound connections (target block) are protected. The server now matches this intent across all four edge operation cases.
  • Minor DB query inefficiency: In the EDGE_OPERATIONS.ADD, EDGES_OPERATIONS.BATCH_REMOVE_EDGES, and EDGES_OPERATIONS.BATCH_ADD_EDGES cases, source block IDs are still included in the connectedBlockIds set and fetched from the database, even though source block protection is no longer checked. These extra fetches are harmless but wasteful.
  • No tests added: The checklist notes tests were not added/updated, which is worth tracking for a behaviour-altering server-side fix.

Confidence Score: 4/5

  • Safe to merge — the change is minimal, well-reasoned, and consistent with the documented client-side design intent.
  • The logic change is small and targeted: four symmetrical removals of the source-block check across the four edge operation paths. It directly mirrors the existing, well-documented client-side isEdgeProtected function. No new dependencies are introduced and no data integrity risk exists since the target-block check is preserved. The only minor concerns are residual unnecessary DB fetches and the absence of automated tests for this server path.
  • No files require special attention beyond the minor DB query inefficiencies noted in apps/sim/socket/database/operations.ts.

Important Files Changed

Filename Overview
apps/sim/socket/database/operations.ts Removes source block protection checks from four edge operation paths (single add, single remove, batch remove, batch add), aligning server-side behavior with the client-side isEdgeProtected utility. Minor: source blocks are still fetched from the DB in the ADD and BATCH_REMOVE/BATCH_ADD cases even though they are no longer used for protection checks.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Edge Operation Request] --> B{Operation Type}

    B -->|ADD| C[Fetch source + target blocks]
    B -->|REMOVE| D[Fetch source + target blocks]
    B -->|BATCH_ADD| E[Fetch all connected blocks]
    B -->|BATCH_REMOVE| F[Fetch all connected blocks]

    C --> G{Is target block protected?}
    D --> H{Is target block protected?}
    E --> I{Filter: any target protected?}
    F --> J{Filter: any target protected?}

    G -->|Yes| K[Skip — log warning]
    G -->|No| L[Insert edge to DB]

    H -->|Yes| M[Skip — log warning]
    H -->|No| N[Delete edge from DB]

    I -->|All protected| O[Skip all — log warning]
    I -->|Some safe| P[Insert safe edges to DB]

    J -->|All protected| Q[Skip all — log warning]
    J -->|Some safe| R[Delete safe edges from DB]

    style K fill:#f99,stroke:#c00
    style M fill:#f99,stroke:#c00
    style O fill:#f99,stroke:#c00
    style Q fill:#f99,stroke:#c00
    style L fill:#9f9,stroke:#090
    style N fill:#9f9,stroke:#090
    style P fill:#9f9,stroke:#090
    style R fill:#9f9,stroke:#090
Loading

Last reviewed commit: 5a2b2ea

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (1)

apps/sim/socket/database/operations.ts
Inconsistent edge removal behavior remains

The EDGE_REMOVE operation still checks both sourceBlockId and targetBlockId for protection (lines 1348–1350), but the client-side uses isEdgeProtected (which only checks the target block) for all edge removal paths. This creates a client/server state divergence: the client will successfully remove an outbound edge from a locked source block (updating its local state), while the server silently discards the operation, leaving them out of sync.

This is the same class of bug just fixed in the EDGE_ADD case. The fix should be applied here as well:

      if (isBlockProtected(edgeToRemove.targetBlockId)) {
        logger.info(`Skipping edge remove - target block is protected`)
        break
      }

@waleedlatif1 waleedlatif1 force-pushed the waleedlatif1/fix-locked-block-connections branch from 2281275 to c42e456 Compare March 3, 2026 20:14
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (1)

apps/sim/socket/database/operations.ts, line 1474
Batch operations not updated to match the single-edge fix

The BATCH_REMOVE_EDGES handler (line 1472) still guards on both source and target blocks being unprotected, which is inconsistent with the fix applied to single-edge REMOVE (line 1348) that only checks the target. The same issue exists in BATCH_ADD_EDGES (line 1571).

This means outbound edges from locked blocks will be silently skipped when added/removed via batch operations, even though the same operations work correctly via single-edge paths. Both filters should only check the target block:

// BATCH_REMOVE_EDGES — should be:
const safeEdgeIds = edgesToRemove
  .filter((e: EdgeToRemove) => !isBlockProtected(e.targetBlockId))
  .map((e: EdgeToRemove) => e.id)

// BATCH_ADD_EDGES — should be:
const safeEdges = (edges as Array<Record<string, unknown>>).filter(
  (e) => !isBlockProtected(e.target as string)
)

@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (2)

apps/sim/socket/database/operations.ts, line 1199
Unnecessary source block fetch after protection check removal

Since the source block's protection is no longer checked, fetching it from the database is unnecessary. The inArray query at line 1199 still includes payload.source, causing a wasted DB read. Only payload.target is needed now.

            inArray(workflowBlocks.id, [payload.target])

apps/sim/socket/database/operations.ts, line 1405
Source block IDs collected but no longer needed

After removing source block protection checks, connectedBlockIds still collects sourceBlockId for every edge (line 1403), which causes extra block records to be fetched from the DB even though they are unused in the subsequent isBlockProtected calls. You can simplify this to only collect target IDs:

      edgesToRemove.forEach((e: EdgeToRemove) => {
        connectedBlockIds.add(e.targetBlockId)
      })

The same applies to the BATCH_ADD_EDGES case around line 1499-1502, where e.source is still added to connectedBlockIds even though source protection is no longer checked.

Copy link

@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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@waleedlatif1 waleedlatif1 merged commit 37bdffe into staging Mar 3, 2026
11 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/fix-locked-block-connections branch March 3, 2026 20:54
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