fix(copilot): mint ids for nested blocks instead of storing the model's handle - #6626
fix(copilot): mint ids for nested blocks instead of storing the model's handle#6626waleedlatif1 wants to merge 2 commits into
Conversation
…'s handle workflow_blocks.id is a global primary key, but normalizeBlockIdsInOperations only minted UUIDs for an operation's own block_id. Children arrive keyed by the model's handle under params.nestedNodes and were persisted verbatim, so a workflow adding a child named "waitPoll" collided with whichever workflow stored that name first and the whole save failed with 23505. The pre-insert delete is scoped to the workflow's own rows, so it cannot clear the conflicting row and every retry fails identically. - claim non-UUID nestedNodes keys recursively, at any container depth - rewrite child connections and nested containers through the same mapping - log the Postgres cause on a failed save; Drizzle's message is only "Failed query: <sql> params: <...>" and the SQLSTATE lives on error.cause
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Failed normalized-table saves also log Reviewed by Cursor Bugbot for commit 7894c0b. Configure here. |
Greptile SummaryThis PR recursively replaces model-generated nested block handles with UUIDs before workflow persistence and rewrites nested connection references through the same mapping. It also logs underlying PostgreSQL error details when normalized workflow persistence fails.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.ts | Recursively claims nested block handles, normalizes nested references, and emits the duplicate-handle warning requested in the prior review. |
| apps/sim/lib/copilot/tools/server/workflow/edit-workflow/builders.test.ts | Adds focused regression tests for UUID minting, recursive containers, sibling connections, and already-valid UUIDs. |
| apps/sim/lib/workflows/persistence/utils.ts | Extends failed-save logging with the underlying error description and PostgreSQL constraint name. |
Reviews (2): Last reviewed commit: "fix(copilot): warn when an edit batch re..." | Re-trigger Greptile
Two declarations sharing a handle collapse to a single block, because references naming that handle are ambiguous and the flat id mapping has no way to express a per-declaration-site id. Behavior is unchanged; the warn makes the case visible in production instead of silent.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 7894c0b. Configure here.
Summary
workflow_blocks.idis a global primary key, butnormalizeBlockIdsInOperationsonly minted UUIDs for an operation's ownblock_id. Children arrive keyed by the model's handle underparams.nestedNodesand were persisted verbatim, so a workflow adding a child namedwaitPollcollided with whichever workflow stored that name first and the entire save failed with23505.nestedNodeskeys recursively at any container depth, and rewrite childconnectionsand nested containers through the same mapping.Failed query: <sql> params: <...>; the SQLSTATE and constraint live onerror.cause, which is why this took a week to diagnose.Root-caused from production CloudWatch: 38 failures today across 3 workflows, and the same signature on Aug 5, 8 and 11 — this is not new. Confirmed against prod: 613 name-like ids across 244 workflows are already stored, each permanently squatting that name (
waitPollsince Jun 10,gmailSendsince Jul 9,buildLeadsince Aug 5). Reproduced against Postgres 17.9:error.cause.code = 23505,detail = Key (id)=(emailWriter) already exists.Type of Change
Testing
3 regression tests added. Verified they fail without the fix (reverted it and watched them go red), then pass with it. Full suite green: 234 tests across the edit-workflow and persistence suites, type-check clean.
Notes / follow-ups
Deliberately scoped. Three things this does not fix:
editops also create children viamergeNestedNodesForParent. Widening the gate would regress: a child matching an existing block by name keeps that block's id, so claiming its handle would repoint sibling references at an id no block was created under. That path needs the id minted at creation with an alias — documented in the TSDoc.apps/realtime/src/database/operations.ts:1004upserts withonConflictDoUpdate({ target: workflowBlocks.id })unscoped by workflow, so a colliding id silently overwrites another workflow's block rather than erroring. Separate bug, worth its own fix.(workflow_id, id)— only two FK dependents (workflow_edges.source_block_id/target_block_id), both already carrying an indexedworkflow_id. That would also close the realtime clobber and make the 613 squatters permanently benign.There is also a second, distinct failure mode on this path affecting all-UUID payloads that this doesn't address; the
error.causelogging is what will identify it.Checklist