fix: comment-frame ids draw from the node-id space (no pin-id collision) (#203)#210
Conversation
…203) `commentNodeId` mapped a comment's stable id into the node-editor id space by offsetting it 2^40 (`comment_id_base`). That base sits INSIDE the pin id space — `pinId` packs `(node_id << 30) | hash`, which reaches 2^40 once a node id hits ~1024 — so a flow with ~1024+ nodes could have a comment frame's node-editor id alias a pin id and bleed drag/selection state. (Rendering-only; persisted `.flow.jsonc` was unaffected.) No bit region is free (pins own bits 0-62, links own bit 63), so allocate comment node-editor ids from the NODE-ID space instead of a separate 2^40-based counter: - Comments now draw their persisted id from the shared `nextNodeId` / `max_node_id` counter (dropped `nextCommentId` / `max_comment_id`), so `commentNodeId(id) == id` — small, below the 2^30 pin floor, and unique vs node ids by construction. - On load, seed `max_node_id` from BOTH node and comment ids, then re-assign any comment id that is 0, a duplicate of another comment (preserves #188/#201 dedup), OR collides with a node id (new #203 constraint). Tests: added a node-id collision reassignment test and a `commentNodeId` vs `pinId`/node-id disjointness assertion; updated the existing #201 comment-id tests to assert against the shared `max_node_id` counter. `zig build`, `zig build test` (465/465) and `zig build gui-test` (26/26) all green.
PR SummaryMedium Risk Overview Shared id counter: Removes Load path: After parsing comments, Editor: Tests cover the shared counter, node–comment collision reassignment, and Reviewed by Cursor Bugbot for commit e54968d. Bugbot is set up for automated code reviews on this repo. Configure here. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request unifies the ID namespace for nodes and comment frames to resolve ID collisions (labelle-gui#203). The separate max_comment_id counter and comment_id_base offset have been removed, and comments now share the max_node_id counter with nodes. The parser has been updated to detect and resolve collisions between comment IDs and node IDs on load, and corresponding unit tests have been added to verify this behavior. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Fixes a flow-editor rendering/interaction bug where comment frame node-editor IDs could collide with pin IDs in large graphs by moving comment frame IDs into the shared node-id space and hardening load-time ID reassignment.
Changes:
- Remove the separate comment-id counter and allocate comment frame IDs from
nextNodeId()/max_node_id. - Update load-time parsing to seed
max_node_idfrom both node + comment IDs and to reassign comment IDs that are0, duplicate, or collide with a node ID. - Extend/adjust unit tests to cover cross-namespace collisions and the new shared-counter behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/tests.zig |
Updates existing comment-id tests for the shared counter and adds coverage for node/comment ID collisions and pin-id disjointness. |
src/modules/flow_doc.zig |
Changes commentNodeId to identity mapping, switches comment creation/repair to use nextNodeId, and exposes helpers used by tests. |
src/flow_io.zig |
Removes max_comment_id/nextCommentId, seeds max_node_id from comments too, and reassigns clashing comment IDs during parse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// id from the SAME counter as nodes (`nextNodeId`/`max_node_id`), so the | ||
| /// identity map below is both small (below the 2^30 pin floor) and unique | ||
| /// against node ids by construction — no separate region, no collision. |
| /// Highest id seen across BOTH nodes and comments — the editor | ||
| /// allocates fresh ids above this. Nodes and comment frames share one | ||
| /// id counter (labelle-gui#203) so a comment's node-editor id | ||
| /// (`commentNodeId(id) == id`) can never collide with a node's id or, | ||
| /// transitively, with a pin id (pins pack `node_id << 30 | hash`). |
| try expect.toBeTrue(doc.comments[0].id > 5); | ||
| } | ||
|
|
||
| test "commentNodeId is disjoint from node ids and pin ids (#203)" { |
Closes #203.
commentNodeIdused base0x100_0000_0000(2^40), which is INSIDE the pin id space (pinId=node_id<<30 | hashreaches 2^40 at node id ~1024), so a comment frame could collide with a pin in a 1024+ node graph (rendering-only — persisted.flow.jsoncwas fine).Fix — shared node-id counter
Comment frames now draw their persisted
idfrom the SAMEnextNodeId/max_node_idcounter as nodes, socommentNodeId(id) == id— small, below the 2^30 pin floor, and unique vs node ids by construction (one counter). The separatenextCommentId/max_comment_id+ 2^40 base are removed. Cleaner than carving a free gap: single id source, disjointness falls out instead of needing a maintained range invariant.Load-time reassignment (
parse): seedmax_node_idfrom BOTH node and comment ids, then reassign a freshnextNodeId()to any comment id that is0, a duplicate (the #188/#201 dedup, preserved), OR equal to a node id (the new #203 cross-namespace constraint).Tests
#201 comment-id tests preserved (assertions moved to the shared counter). New: a comment id colliding with a node id is reassigned on load;
commentNodeId(n)is disjoint frompinIdfor node ids {1,1024,2048,100000}.zig build test465/465,gui-test26/26 (agent run; CI confirms).