Skip to content

fix: comment-frame ids draw from the node-id space (no pin-id collision) (#203) #210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
apotema merged 1 commit into from
Jun 9, 2026
Merged

fix: comment-frame ids draw from the node-id space (no pin-id collision) (#203) #210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 33 additions & 29 deletions src/flow_io.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`).
Comment on lines +750 to +754
/// 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;
Expand All @@ -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 ─────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -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();
}
}

Expand Down
43 changes: 24 additions & 19 deletions src/modules/flow_doc.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +1644 to +1646
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;
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
69 changes: 62 additions & 7 deletions src/tests.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
\\{
Expand All @@ -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);
Expand Down Expand Up @@ -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)" {
Expand Down Expand Up @@ -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
Expand Down
Loading