[Birfy]

Written-by: Dev

Fixes #2663
Owner: Dev
Reviewer: Reviewer
Docs-not-needed: node-id allocation fix in the New workflow dialog, no architecture change (#2663)

What changed

CreateModal.add in plugins/workflow/client.js allocated a new node's id from the layer count (n${list.length + 1}). A Parallel step joins the last layer, so the count stops growing and the third Add a Bot produced n2 again; validateGraph in plugins/workflow/src/graph.js then refused the whole Create with Node n2 is listed twice.

The id now comes from nextNodeId(list.flat()), the same collision-aware helper the canvas drop path already uses at client.js:1232. One call site changes: nextNodeId itself, the canvas drop path and graph.js are untouched.

Tests in plugins/workflow/test/client.test.js drive the real dialog through the hook renderer: under Parallel, three Add a Bot clicks produce three distinct ids, one flat layer, and a graph validateGraph accepts; with Parallel unticked the ids stay n1,n2,n3 with one layer each.

Changelog: changelog.d/2663-parallel-node-id.fixed.md.

Verification

Red first — source reverted to main with the new tests kept: not ok 136 … three Bots in one Parallel step got n1,n2,n2, # pass 176 / fail 1.

Ran at this tip: cae6bd0c0e27095d1914ed8752e5f0db9c0c8388 (tarball of that commit, byte-identical to the working copy for the three changed paths).

$ diff -rq <pristine main tarball> .    # only the three files below differ
$ cd plugins/workflow && TZ=UTC node --import ./test/resolve.mjs --test test/client.test.js

Result: 177 tests, 177 pass, 0 fail, including both #2663 cases (136 and 137).

Not run: npm run verify and the full plugin suite — the brief says not to run it while other work is in parallel, and this diff touches one test file. node .github/scripts/changelog.mjs check ran in the implementation step and parsed the fragments; not re-run here.

Checkpoint

{
  "pr": 2665,
  "claim": "Dev",
  "issue": 2663,
  "summary": "CreateModal.add numbered each new node from the layer count, so a third Bot added to a Parallel step reused the second Bot's id and Create failed with `Node n2 is listed twice`. It now allocates over the flat node list with nextNodeId, the same helper the canvas drop path uses. Two tests drive the real dialog: Parallel gives three distinct ids that validateGraph accepts, and the sequential path still numbers n1, n2, n3. The change is confined to this dialog; nextNodeId, the canvas drop path and graph.js are untouched.",
  "files": [
    "plugins/workflow/client.js",
    "plugins/workflow/test/client.test.js",
    "changelog.d/2663-parallel-node-id.fixed.md"
  ],
  "checks": {
    "ran": "cd plugins/workflow && TZ=UTC node --import ./test/resolve.mjs --test test/client.test.js",
    "result": "green",
    "at": "cae6bd0c0e27095d1914ed8752e5f0db9c0c8388"
  },
  "next": "none — ready for review"
}

[Birfy]

Verdict: PASS at cae6bd0
Written-by: Reviewer
Attestation: cae6bd0 role=Reviewer

Ran at this tip:

$ cd plugins/workflow && TZ=UTC node --import ./test/resolve.mjs --test test/client.test.js
# tests 177  # pass 177  # fail 0
$ cd plugins/workflow && TZ=UTC node --import ./test/resolve.mjs --test test/client.test.js
# tests 177  # pass 176  # fail 1
$ cd plugins/workflow && TZ=UTC node --import ./test/resolve.mjs --test test/client.test.js
# tests 177  # pass 177  # fail 0

The three runs above are, in order: the tip as pushed (green, 177/177); the tip with only the CreateModal.add id line put back to the base blob d8a896c4 form (id: \n${list.length + 1}`) and the new tests kept — red first, not ok 136 … three Bots in one Parallel step got n1,n2,n2, # pass 176 / fail 1; then the tip restored (green, 177/177). Fetched as the cae6bd0` tarball, nothing else touched.

Review points:

• plugins/workflow/client.js:1604-1606 — one line in CreateModal.add now allocates with nextNodeId(list.flat()), plus the two-line comment. nextNodeId itself (client.js:683-688), the canvas drop path (client.js:1232) and plugins/workflow/src/graph.js are untouched; compare d8a896c4...cae6bd0c lists exactly the three files of this change and the tip's only parent is the base.
• Parallel path: three Bots added with Parallel ticked produce three distinct ids in one flat layer and validateGraph accepts the body that reaches workflow/create (workflow: Parallel mode in the New workflow dialog gives the third "Add a Bot" a duplicate node id, so Create is refused #2663 test 136). Sequential path unchanged: n1,n2,n3, one layer each (test 137). Both cases are asserted, not just the fixed one.
• nextNodeId read, not only exercised: it starts at nodes.length + 1 and increments while the id is in the set, so a list with gaps or out-of-order ids still gets a free id.
• Range kept: graph.js's Node n2 is listed twice refusal is untouched (it is correct behaviour, not the bug); no rename or extraction of nextNodeId; the only line removed from the old test file is the cleanTools import, widened to { cleanTools, validateGraph } — no existing assertion weakened to go green.
• Docs-not-needed: is present in the PR body and in the tip commit message.
• Checkpoint block is compliant: 1039 chars total, summary 523, files 3 (one in this diff), checks.at the full 40-char tip.
• check-runs at the tip: only enqueue-guard red (no Verifier report before this one); tests-ok, Node 24 · DSH 0.2.0-rc.2, plugins (workflow/tree/social/timeline), package, policy-sign, merge-gate all success, mock/live/window skipped or success.