diff --git a/src/flow_io.zig b/src/flow_io.zig index c6803f1..aeb519c 100644 --- a/src/flow_io.zig +++ b/src/flow_io.zig @@ -747,13 +747,13 @@ pub const FlowDoc = struct { /// only when non-empty so pre-comment files round-trip byte-for-byte. /// flow-codegen ignores the key, so it has no codegen effect. comments: []Comment = &.{}, - /// Highest node id seen — the editor allocates fresh ids above this. + /// 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`). + /// Seeded on load from every node *and* comment id; see `parse`. max_node_id: u32 = 0, - /// Highest comment id seen (labelle-gui#188). Comment ids live in their - /// own namespace (they're not node ids), so they get their own - /// monotonic counter. Seeded from the loaded `comments` block and - /// bumped by `nextCommentId` so a fresh frame never reuses an id. - max_comment_id: u32 = 0, pub fn deinit(self: *FlowDoc) void { const child = self.arena.child_allocator; @@ -765,21 +765,14 @@ pub const FlowDoc = struct { return self.arena.allocator(); } - /// Allocate a fresh node id one above the current maximum. Bumps - /// `max_node_id` so successive calls don't collide. + /// Allocate a fresh id one above the current maximum, used for both + /// nodes and comment frames (labelle-gui#203 — shared namespace). Bumps + /// `max_node_id` so successive calls don't collide. Ids are 1-based; + /// `0` stays reserved as the "unassigned" sentinel. pub fn nextNodeId(self: *FlowDoc) u32 { self.max_node_id += 1; return self.max_node_id; } - - /// Allocate a fresh comment id one above the current maximum - /// (labelle-gui#188). Bumps `max_comment_id` so successive calls — and - /// any id-on-load assignment — never collide. Ids are 1-based; `0` - /// stays reserved as the "unassigned" sentinel. - pub fn nextCommentId(self: *FlowDoc) u32 { - self.max_comment_id += 1; - return self.max_comment_id; - } }; // ─── Reader ───────────────────────────────────────────────────────────── @@ -887,25 +880,36 @@ pub fn parse(child_allocator: std.mem.Allocator, raw: []const u8) !FlowDoc { if (root.get("comments")) |v| { if (v != .array) return ParseError.BadSchema; doc.comments = try parseComments(a, v.array); - // Seed the comment-id counter from the highest persisted id, then - // backfill ids for any pre-id / unassigned (`0`) entries so every - // comment carries a stable, unique editor id (labelle-gui#188). + // Comment frames share the node id counter (labelle-gui#203): + // `commentNodeId(id) == id`, so a comment id must be unique against + // node ids as well as other comment ids. `max_node_id` was already + // seeded from the node ids above; fold the persisted comment ids in + // too so a fresh `nextNodeId()` lands strictly above every existing + // id, then backfill / re-assign as needed. for (doc.comments) |c| { - if (c.id > doc.max_comment_id) doc.max_comment_id = c.id; + if (c.id > doc.max_node_id) doc.max_node_id = c.id; } - // Assign a fresh id to any unassigned (`0`) OR duplicate entry — - // two comments sharing an id would alias to a single node-editor - // frame and bleed drag/resize/label state. `max_comment_id` is - // seeded above, so `nextCommentId()` can't collide (bugbot). + // Re-assign a fresh id to any comment whose id is unassigned (`0`), + // duplicates another comment (labelle-gui#188 / #201), OR collides + // with a NODE id (labelle-gui#203) — any of these would alias two + // frames (or a frame and a node) onto one node-editor id and bleed + // drag/resize/label/selection state. `max_node_id` is seeded above, + // so `nextNodeId()` can't collide with anything already present. for (doc.comments, 0..) |*c, i| { - var dup = c.id == 0; - if (!dup) for (doc.comments[0..i]) |prev| { + var clash = c.id == 0; + if (!clash) for (doc.comments[0..i]) |prev| { if (prev.id == c.id) { - dup = true; + clash = true; + break; + } + }; + if (!clash) for (doc.nodes) |n| { + if (n.id == c.id) { + clash = true; break; } }; - if (dup) c.id = doc.nextCommentId(); + if (clash) c.id = doc.nextNodeId(); } } diff --git a/src/modules/flow_doc.zig b/src/modules/flow_doc.zig index 7ad25da..cf7a477 100644 --- a/src/modules/flow_doc.zig +++ b/src/modules/flow_doc.zig @@ -1629,24 +1629,26 @@ fn linkId(e: flow_io.Edge) u64 { return (h.final() & 0x7FFF_FFFF_FFFF_FFFF) | (@as(u64, 1) << 63); } -/// Base for comment-frame node-editor ids (labelle-gui#188). Comment -/// frames are rendered as imgui-node-editor *group* nodes so the editor -/// drags/resizes them natively, but they're not `flow_io.Node`s — they -/// need ids in the same `NodeId` space that can't collide with a real -/// node's id (`@intCast(n.id)`, a small `u32`). The high base keeps the -/// two spaces disjoint: real node ids are small, comment ids start near -/// 2^40, so no realistic node count reaches them. -const comment_id_base: u64 = 0x100_0000_0000; - /// Map a comment's *stable* id (`Comment.id`, labelle-gui#188) into the /// node-editor's id namespace. Keyed by the stable id — never the slice /// index — so deleting a non-last comment doesn't shift surviving frames /// onto another frame's editor (drag/resize) state. -fn commentNodeId(comment_id: u32) u64 { - return comment_id_base + @as(u64, comment_id); +/// +/// Comment frames are rendered as imgui-node-editor *group* nodes so the +/// editor drags/resizes them natively, but they're not `flow_io.Node`s — +/// they need `NodeId`s that can't collide with a real node's id +/// (`@intCast(n.id)`) OR a pin id (`pinId` packs `node_id << 30 | hash`, +/// reaching 2^40 once a node id hits ~1024). The old scheme offset comment +/// ids by 2^40, which sits *inside* that pin space and collided in flows +/// with ~1024+ nodes (labelle-gui#203). Instead, comments now draw their +/// 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. +pub fn commentNodeId(comment_id: u32) u64 { + return @as(u64, comment_id); } -const PinDir = enum { input, output }; +pub const PinDir = enum { input, output }; /// A frame-scoped set of pin names, used to de-duplicate the pins a /// `Subflow` node emits within one direction. Keyed by the name bytes; @@ -1660,7 +1662,7 @@ const PinNameSet = std.StringHashMap(void); /// whole pin-id space stays disjoint from link ids (`linkId` always /// sets bit 63). Collisions within the name-hash bits are astronomically /// unlikely and only cost a mis-drawn link, never data loss. -fn pinId(node: u32, name: []const u8, dir: PinDir) u64 { +pub fn pinId(node: u32, name: []const u8, dir: PinDir) u64 { var h = std.hash.Wyhash.init(node); h.update(name); const base = (h.final() & 0x3FFF_FFFF) | (@as(u64, node) << 30); @@ -1698,8 +1700,9 @@ fn renderComments(s: *FlowDocState) void { // A defensively-assigned id for any frame that somehow reached the // canvas without one (id `0` is the "unassigned" sentinel). The // loader and `appendComment` both assign ids, so this is belt-and- - // braces — but a `0` id would alias `comment_id_base` across frames. - if (c.id == 0) c.id = s.doc.nextCommentId(); + // braces — but a `0` id would map every such frame onto node-editor + // id 0 and bleed editor state. Shares the node id counter (#203). + if (c.id == 0) c.id = s.doc.nextNodeId(); const id = commentNodeId(c.id); // Seed the editor's position from the doc the first frame, the @@ -3946,10 +3949,12 @@ fn appendComment(s: *FlowDocState) !void { .y = 24 + offset, .w = 220, .h = 140, - // Stable editor id (labelle-gui#188) — mirror how nodes get - // `nextNodeId()`. Persisted so it survives loads and never aliases - // another frame's editor state after a sibling delete. - .id = s.doc.nextCommentId(), + // Stable editor id (labelle-gui#188) — drawn from the shared node + // id counter (`nextNodeId`, labelle-gui#203) so the comment's + // node-editor id can't collide with a node's id or a pin id. + // Persisted so it survives loads and never aliases another frame's + // editor state after a sibling delete. + .id = s.doc.nextNodeId(), }; s.doc.comments = try growComments(a, s.doc.comments, c); s.needs_layout = true; // re-seed so the new frame's pos is applied diff --git a/src/tests.zig b/src/tests.zig index dbf0cbc..e292278 100644 --- a/src/tests.zig +++ b/src/tests.zig @@ -5513,11 +5513,12 @@ pub const FlowIoTests = struct { // ── comment stable ids (labelle-gui#188, bugbot: delete reuses ids) ── - test "comment id round-trips and seeds max_comment_id" { + test "comment id round-trips and seeds shared max_node_id" { // A persisted `id` survives a save/load so the editor's stable // node-editor key is reproducible across sessions, and the loaded - // doc's `max_comment_id` is the highest id seen (so the next fresh - // comment never reuses one). + // doc's `max_node_id` (shared with nodes, labelle-gui#203) folds in + // the highest comment id (so the next fresh node/comment never + // reuses one). const a = std.testing.allocator; const src = \\{ @@ -5535,9 +5536,11 @@ pub const FlowIoTests = struct { try expect.equal(doc.comments.len, @as(usize, 2)); try expect.equal(doc.comments[0].id, @as(u32, 3)); try expect.equal(doc.comments[1].id, @as(u32, 7)); - try expect.equal(doc.max_comment_id, @as(u32, 7)); + // Comment ids fold into the shared node-id counter (#203); with no + // nodes present the highest comment id (7) is the max. + try expect.equal(doc.max_node_id, @as(u32, 7)); // A fresh id lands strictly above the highest persisted one. - try expect.equal(doc.nextCommentId(), @as(u32, 8)); + try expect.equal(doc.nextNodeId(), @as(u32, 8)); const text = try flow_io.render(a, doc); defer a.free(text); @@ -5573,9 +5576,10 @@ pub const FlowIoTests = struct { try expect.toBeTrue(doc.comments[0].id != 0); try expect.toBeTrue(doc.comments[1].id != 0); try expect.toBeTrue(doc.comments[0].id != doc.comments[1].id); - // …and the counter is consistent with what was assigned. + // …and the shared counter (#203) is consistent with what was + // assigned. const max_assigned = @max(doc.comments[0].id, doc.comments[1].id); - try expect.equal(doc.max_comment_id, max_assigned); + try expect.equal(doc.max_node_id, max_assigned); } test "duplicate comment ids are de-aliased on load (bugbot)" { @@ -5624,6 +5628,57 @@ pub const FlowIoTests = struct { try expect.toBeTrue(doc.comments[1].id > 5); } + test "comment id colliding with a node id is reassigned on load (#203)" { + // labelle-gui#203: comment node-editor ids share the node id space + // (`commentNodeId(id) == id`), so a persisted comment id that equals + // a NODE id would alias the comment frame onto that node's editor + // entry and bleed drag/selection state. The loader must detect the + // cross-collision and hand the comment a fresh, distinct id. + const a = std.testing.allocator; + const src = + \\{ + \\ "event": { "type": "OnCreate" }, + \\ "nodes": [ + \\ { "id": 5, "type": "Log", "params": {} } + \\ ], + \\ "edges": [], + \\ "comments": [ + \\ { "id": 5, "text": "collides with node 5", "x": 0, "y": 0, "w": 200, "h": 120 } + \\ ] + \\} + ; + var doc = try flow_io.parse(a, src); + defer doc.deinit(); + try expect.equal(doc.nodes.len, @as(usize, 1)); + try expect.equal(doc.comments.len, @as(usize, 1)); + // The node keeps its id; the comment is moved off the collision. + try expect.equal(doc.nodes[0].id, @as(u32, 5)); + try expect.toBeTrue(doc.comments[0].id != 5); + try expect.toBeTrue(doc.comments[0].id != 0); + // And the reassigned id is above the shared max (node id 5). + try expect.toBeTrue(doc.comments[0].id > 5); + } + + test "commentNodeId is disjoint from node ids and pin ids (#203)" { + // The core invariant behind the #203 fix: because comment frames + // draw their id from the same counter as nodes, mapping a comment + // id through `commentNodeId` yields the id unchanged (a small u32), + // which is necessarily distinct from any other node's id and from + // every pin id (`pinId` packs `node_id << 30 | hash`, whose value is + // always >= node_id << 30 >= 2^30 for any non-zero node). A comment + // id stays a small integer well below that pin floor. + // Identity map: commentNodeId(id) == id. + try expect.equal(flow_doc.commentNodeId(7), @as(u64, 7)); + // A comment id (here 7, from the shared counter) can't equal any + // pin id, even for a high-numbered node that previously reached the + // old 2^40 comment base. Bit 30+ is always set on a pin id. + const comment = flow_doc.commentNodeId(7); + inline for (.{ 1, 1024, 2048, 100000 }) |node| { + try expect.toBeTrue(comment != flow_doc.pinId(node, "in", .input)); + try expect.toBeTrue(comment != flow_doc.pinId(node, "out", .output)); + } + } + test "deleting comment[0] leaves comment[1] id+geometry intact" { // Model-level reproduction of the bugbot "delete reuses editor ids" // bug. Deleting a non-last comment shifts slice indices; with a