From 3672ffb4e9eba0a2a15a955f702367f6d30c5178 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Sat, 11 Jul 2026 10:04:41 +0800 Subject: [PATCH] perf(figma): cut import clone amplification Decoded node changes were held twice (cloned into by_key, cloned again into TreeNodes), every symbol subtree was deep-cloned per component instance, and every embedded blob was eagerly base64-encoded whether referenced or not. Materialization now moves out of by_key via a deduplicated latest-wins adjacency (repeated GUID records collapse to one edge), symbol prototypes are shared via Rc, and blob encoding is lazy and memoized so unreferenced blobs are never encoded. --- crates/op-figma/src/common.rs | 7 +- crates/op-figma/src/converters.rs | 10 +- crates/op-figma/src/image_resolver.rs | 192 +++++++++++++----- crates/op-figma/src/instance/assignment.rs | 3 +- .../op-figma/src/instance/assignment_tests.rs | 11 +- crates/op-figma/src/node_mapper.rs | 7 +- crates/op-figma/src/tree.rs | 141 ++++++++++--- 7 files changed, 277 insertions(+), 94 deletions(-) diff --git a/crates/op-figma/src/common.rs b/crates/op-figma/src/common.rs index b7443efac..f20f0da10 100644 --- a/crates/op-figma/src/common.rs +++ b/crates/op-figma/src/common.rs @@ -11,6 +11,7 @@ use jian_ops_schema::node::base::{NumberOrExpression, PenNodeBase}; use jian_ops_schema::node::container::CornerRadius; use jian_ops_schema::sizing::SizingBehavior; use std::collections::HashMap; +use std::rc::Rc; use std::sync::RwLock; /// Stroke vs fill rendering for a host-resolved icon. @@ -94,8 +95,10 @@ pub const SKIPPED_TYPES: &[&str] = &[ pub struct ConversionContext { /// Figma SYMBOL guid → minted `fig_N` ref id. pub component_map: HashMap, - /// Figma SYMBOL guid → its subtree (for instance inlining). - pub symbol_tree: HashMap, + /// Figma SYMBOL guid → its subtree (for instance inlining). `Rc`-shared + /// so every instance of the same SYMBOL reuses one clone instead of + /// paying for a fresh deep clone per instance. + pub symbol_tree: HashMap>, /// Non-fatal conversion notes. pub warnings: Vec, /// Sequential `fig_N` id allocator. diff --git a/crates/op-figma/src/converters.rs b/crates/op-figma/src/converters.rs index 4b11757cf..a6a63f5ba 100644 --- a/crates/op-figma/src/converters.rs +++ b/crates/op-figma/src/converters.rs @@ -285,7 +285,11 @@ fn convert_instance( if inline { let key = component_guid.clone().unwrap(); - if let Some(symbol_node) = ctx.symbol_tree.get(&key).cloned() { + // Borrow the `Rc` in place instead of cloning it — the + // symbol map and `instance_assignments` are disjoint fields of + // `ctx`, so this borrow coexists with the `&mut` below without + // needing an owned copy of the (potentially large) subtree. + if let Some(symbol_node) = ctx.symbol_tree.get(&key) { if !symbol_node.children.is_empty() { let overrides = figma .get("symbolData") @@ -299,13 +303,13 @@ fn convert_instance( let is_swap = figma.get("overriddenSymbolID").is_some(); let derived = figma.get_array("derivedSymbolData").map(|a| { if is_swap { - crate::instance::filter_swap_stale_derived(a, &symbol_node, instance_size) + crate::instance::filter_swap_stale_derived(a, symbol_node, instance_size) } else { a.to_vec() } }); let children = apply_instance_overrides_cached( - &symbol_node, + symbol_node, overrides.as_deref(), derived.as_deref(), instance_size, diff --git a/crates/op-figma/src/image_resolver.rs b/crates/op-figma/src/image_resolver.rs index 4091c06b6..3caf46683 100644 --- a/crates/op-figma/src/image_resolver.rs +++ b/crates/op-figma/src/image_resolver.rs @@ -7,6 +7,7 @@ use jian_ops_schema::document::PenDocument; use jian_ops_schema::node::PenNode; use jian_ops_schema::style::PenFill; use std::collections::HashMap; +use std::rc::Rc; /// Encode raw image bytes as a `data:` URL, sniffing the MIME type /// from the leading magic bytes (PNG fallback). @@ -21,34 +22,77 @@ fn blob_to_data_url(bytes: &[u8]) -> String { format!("data:{mime};base64,{b64}") } -/// Resolve a `__blob:` / `__hash:` placeholder to a data URL. -fn resolve_ref( - src: &str, - data_urls: &HashMap, - hash_urls: &HashMap, -) -> Option { - if let Some(rest) = src.strip_prefix("__blob:") { - let index: u32 = rest.parse().ok()?; - return data_urls.get(&index).cloned(); - } - if let Some(hash) = src.strip_prefix("__hash:") { - return hash_urls.get(hash).cloned(); - } - None +/// Lazy, memoized `__blob:` / `__hash:` → data-URL resolver. Encoding +/// (base64, which allocates + copies the whole payload) is deferred +/// until a fill actually references the blob, and the result is cached +/// as a shared `Rc` — a never-referenced blob is never encoded at +/// all, and a blob referenced by several fills is encoded exactly once, +/// with every subsequent reference sharing the same allocation via a +/// cheap refcount bump instead of a fresh `String` copy. +struct BlobCache<'a> { + image_blobs: &'a HashMap>, + image_files: &'a HashMap>, + blob_urls: HashMap>, + hash_urls: HashMap>, + /// Number of times a blob was actually base64-encoded (cache + /// misses only) — exposed for tests that verify the memoization + + /// never-referenced-blob-skipped invariants. + encode_calls: u32, } -fn patch_fills( - fills: &mut Option>, - data_urls: &HashMap, - hash_urls: &HashMap, -) -> usize { +impl<'a> BlobCache<'a> { + fn new( + image_blobs: &'a HashMap>, + image_files: &'a HashMap>, + ) -> Self { + Self { + image_blobs, + image_files, + blob_urls: HashMap::new(), + hash_urls: HashMap::new(), + encode_calls: 0, + } + } + + /// Resolve a `__blob:` / `__hash:` placeholder to a shared data + /// URL, encoding + caching on first reference only. + fn resolve(&mut self, src: &str) -> Option> { + if let Some(rest) = src.strip_prefix("__blob:") { + let index: u32 = rest.parse().ok()?; + if let Some(cached) = self.blob_urls.get(&index) { + return Some(Rc::clone(cached)); + } + let bytes = self.image_blobs.get(&index)?; + let url: Rc = Rc::from(blob_to_data_url(bytes)); + self.encode_calls += 1; + self.blob_urls.insert(index, Rc::clone(&url)); + return Some(url); + } + if let Some(hash) = src.strip_prefix("__hash:") { + if let Some(cached) = self.hash_urls.get(hash) { + return Some(Rc::clone(cached)); + } + let bytes = self.image_files.get(hash)?; + let url: Rc = Rc::from(blob_to_data_url(bytes)); + self.encode_calls += 1; + self.hash_urls.insert(hash.to_string(), Rc::clone(&url)); + return Some(url); + } + None + } +} + +fn patch_fills(fills: &mut Option>, cache: &mut BlobCache) -> usize { let mut count = 0; if let Some(fills) = fills { for fill in fills { if let PenFill::Image(img) = fill { if img.url.starts_with("__blob:") || img.url.starts_with("__hash:") { - if let Some(url) = resolve_ref(&img.url, data_urls, hash_urls) { - img.url = url.into(); + if let Some(url) = cache.resolve(&img.url) { + // Materialize the owned `ImageSrc` only here, at + // the schema boundary — everywhere upstream the + // resolved URL travels as a shared `Rc`. + img.url = (&*url).into(); count += 1; } } @@ -59,39 +103,35 @@ fn patch_fills( } /// Patch one node's image fills, then recurse into its children. -fn patch_node( - node: &mut PenNode, - data_urls: &HashMap, - hash_urls: &HashMap, -) -> usize { +fn patch_node(node: &mut PenNode, cache: &mut BlobCache) -> usize { let mut count = 0; let children: Option<&mut Vec> = match node { PenNode::Frame(f) => { - count += patch_fills(&mut f.container.fill, data_urls, hash_urls); + count += patch_fills(&mut f.container.fill, cache); f.children.as_mut() } PenNode::Group(g) => { - count += patch_fills(&mut g.container.fill, data_urls, hash_urls); + count += patch_fills(&mut g.container.fill, cache); g.children.as_mut() } PenNode::Rectangle(r) => { - count += patch_fills(&mut r.container.fill, data_urls, hash_urls); + count += patch_fills(&mut r.container.fill, cache); r.children.as_mut() } PenNode::Ellipse(e) => { - count += patch_fills(&mut e.fill, data_urls, hash_urls); + count += patch_fills(&mut e.fill, cache); None } PenNode::Polygon(p) => { - count += patch_fills(&mut p.fill, data_urls, hash_urls); + count += patch_fills(&mut p.fill, cache); None } PenNode::Path(p) => { - count += patch_fills(&mut p.fill, data_urls, hash_urls); + count += patch_fills(&mut p.fill, cache); None } PenNode::Text(t) => { - count += patch_fills(&mut t.fill, data_urls, hash_urls); + count += patch_fills(&mut t.fill, cache); None } PenNode::Ref(r) => r.children.as_mut(), @@ -99,7 +139,7 @@ fn patch_node( }; if let Some(children) = children { for child in children { - count += patch_node(child, data_urls, hash_urls); + count += patch_node(child, cache); } } count @@ -115,25 +155,18 @@ pub fn resolve_image_blobs( if image_blobs.is_empty() && image_files.is_empty() { return 0; } - let data_urls: HashMap = image_blobs - .iter() - .map(|(k, v)| (*k, blob_to_data_url(v))) - .collect(); - let hash_urls: HashMap = image_files - .iter() - .map(|(k, v)| (k.clone(), blob_to_data_url(v))) - .collect(); + let mut cache = BlobCache::new(image_blobs, image_files); let mut count = 0; if let Some(pages) = &mut doc.pages { for page in pages { for child in &mut page.children { - count += patch_node(child, &data_urls, &hash_urls); + count += patch_node(child, &mut cache); } } } for child in &mut doc.children { - count += patch_node(child, &data_urls, &hash_urls); + count += patch_node(child, &mut cache); } count } @@ -152,24 +185,73 @@ mod tests { assert!(blob_to_data_url(&[0xFF, 0xD8, 0xFF, 0xE0]).starts_with("data:image/jpeg;base64,")); } + fn png_bytes() -> Vec { + vec![0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0xAA] + } + #[test] fn resolve_blob_ref() { - let mut data_urls = HashMap::new(); - data_urls.insert(3u32, "data:image/png;base64,AAA".to_string()); - assert_eq!( - resolve_ref("__blob:3", &data_urls, &HashMap::new()).as_deref(), - Some("data:image/png;base64,AAA") - ); - assert!(resolve_ref("__blob:9", &data_urls, &HashMap::new()).is_none()); + let mut blobs = HashMap::new(); + blobs.insert(3u32, png_bytes()); + let no_files = HashMap::new(); + let mut cache = BlobCache::new(&blobs, &no_files); + let resolved = cache.resolve("__blob:3").expect("blob 3 resolves"); + assert!(resolved.starts_with("data:image/png;base64,")); + assert!(cache.resolve("__blob:9").is_none()); } #[test] fn resolve_hash_ref() { - let mut hash_urls = HashMap::new(); - hash_urls.insert("abcd".to_string(), "data:image/png;base64,ZZ".to_string()); + let mut files = HashMap::new(); + files.insert("abcd".to_string(), png_bytes()); + let no_blobs = HashMap::new(); + let mut cache = BlobCache::new(&no_blobs, &files); + let resolved = cache.resolve("__hash:abcd").expect("hash abcd resolves"); + assert!(resolved.starts_with("data:image/png;base64,")); + } + + /// A blob that no fill ever references must never be run through + /// `blob_to_data_url` — the whole point of laziness is to skip the + /// (potentially large) encode for image-heavy files where most + /// blobs aren't actually used by any visible fill. + #[test] + fn unreferenced_blob_is_never_encoded() { + let mut blobs = HashMap::new(); + blobs.insert(0u32, png_bytes()); + blobs.insert(1u32, png_bytes()); + let no_files = HashMap::new(); + let mut cache = BlobCache::new(&blobs, &no_files); + + cache.resolve("__blob:0").expect("blob 0 resolves"); + + assert_eq!(cache.encode_calls, 1, "only the referenced blob is encoded"); + assert!(cache.blob_urls.contains_key(&0)); + assert!( + !cache.blob_urls.contains_key(&1), + "blob 1 was never referenced and must not be cached/encoded" + ); + } + + /// Two fills pointing at the same blob must share the encoding + /// work: the base64 pass runs once, and both callers get the same + /// `Rc` allocation back (a refcount bump, not a fresh string copy). + #[test] + fn shared_blob_is_encoded_exactly_once() { + let mut blobs = HashMap::new(); + blobs.insert(0u32, png_bytes()); + let no_files = HashMap::new(); + let mut cache = BlobCache::new(&blobs, &no_files); + + let first = cache.resolve("__blob:0").expect("first fill resolves"); + let second = cache.resolve("__blob:0").expect("second fill resolves"); + assert_eq!( - resolve_ref("__hash:abcd", &HashMap::new(), &hash_urls).as_deref(), - Some("data:image/png;base64,ZZ") + cache.encode_calls, 1, + "encode runs once despite two references" + ); + assert!( + Rc::ptr_eq(&first, &second), + "both references share the same Rc allocation" ); } } diff --git a/crates/op-figma/src/instance/assignment.rs b/crates/op-figma/src/instance/assignment.rs index d9759878f..8a5beafe4 100644 --- a/crates/op-figma/src/instance/assignment.rs +++ b/crates/op-figma/src/instance/assignment.rs @@ -4,6 +4,7 @@ use crate::figma_types::FigVec2; use crate::kiwi::FigValue; use crate::tree::{guid_to_string, TreeNode}; use std::collections::HashMap; +use std::rc::Rc; /// Pre-conversion pooled seeding: walk the whole tree, group every /// INSTANCE's single-segment virtual entries by SYMBOL, pool their @@ -12,7 +13,7 @@ use std::collections::HashMap; /// pin quality doesn't depend on which instance converts first. pub fn seed_assignments_from_instances( root: &TreeNode, - symbol_tree: &HashMap, + symbol_tree: &HashMap>, cache: &mut HashMap, ) { // symbol guid -> pk -> pooled evidence. diff --git a/crates/op-figma/src/instance/assignment_tests.rs b/crates/op-figma/src/instance/assignment_tests.rs index 981932583..9df449253 100644 --- a/crates/op-figma/src/instance/assignment_tests.rs +++ b/crates/op-figma/src/instance/assignment_tests.rs @@ -2,6 +2,7 @@ //! main instance strategy matrix to keep each test module small. use super::*; +use std::rc::Rc; fn obj(pairs: Vec<(&str, FigValue)>) -> FigValue { FigValue::Object(pairs.into_iter().map(|(k, v)| (k.to_string(), v)).collect()) @@ -147,7 +148,7 @@ fn pooled_seeding_assigns_by_cross_instance_evidence() { text_leaf("delta", 12, "+0.00%", 40.0, 14.0), ]); let mut symbol_tree = HashMap::new(); - symbol_tree.insert("0:0".to_string(), sym); + symbol_tree.insert("0:0".to_string(), Rc::new(sym)); let make_instance = |lid: u32, ov: Vec, dv: Vec| TreeNode { figma: obj(vec![ @@ -243,7 +244,7 @@ fn geometry_seeding_pins_from_rich_sibling_instance() { ]) }; let mut symbol_tree = HashMap::new(); - symbol_tree.insert("0:0".to_string(), make_sym()); + symbol_tree.insert("0:0".to_string(), Rc::new(make_sym())); let make_instance = |lid: u32, ov: Vec, dv: Vec| TreeNode { figma: obj(vec![ @@ -369,7 +370,7 @@ fn geometry_seeding_uses_fill_override_as_tie_breaker() { ]) }; let mut symbol_tree = HashMap::new(); - symbol_tree.insert("0:0".to_string(), make_sym()); + symbol_tree.insert("0:0".to_string(), Rc::new(make_sym())); let rich = TreeNode { figma: obj(vec![ @@ -455,7 +456,7 @@ fn swapped_instance_does_not_seed_base_component_cache() { make_leaf("accent", 11, 8.0, 8.0, 11.0), ]); let mut symbol_tree = HashMap::new(); - symbol_tree.insert("0:0".to_string(), base_sym); + symbol_tree.insert("0:0".to_string(), Rc::new(base_sym)); let geom = |pk_lid: u32, w: f32, h: f32, x: f32| { ov_with( @@ -517,7 +518,7 @@ fn pooled_seeding_does_not_overwrite_existing_pin() { text_leaf("value", 11, "0", 20.0, 24.0), ]); let mut symbol_tree = HashMap::new(); - symbol_tree.insert("0:0".to_string(), sym); + symbol_tree.insert("0:0".to_string(), Rc::new(sym)); let make_root = || TreeNode { figma: obj(vec![ diff --git a/crates/op-figma/src/node_mapper.rs b/crates/op-figma/src/node_mapper.rs index f3119de7d..d2f77f880 100644 --- a/crates/op-figma/src/node_mapper.rs +++ b/crates/op-figma/src/node_mapper.rs @@ -14,6 +14,7 @@ use jian_ops_schema::document::PenDocument; use jian_ops_schema::node::PenNode; use jian_ops_schema::page::PenPage; use std::collections::HashMap; +use std::rc::Rc; /// Outcome of a full-document Figma import. pub struct FigmaImportResult { @@ -212,7 +213,7 @@ pub fn figma_all_pages_to_pen_document( } let mut component_map: HashMap = HashMap::new(); - let mut symbol_tree: HashMap = HashMap::new(); + let mut symbol_tree: HashMap> = HashMap::new(); let mut counter: u32 = 1; for page in &pages { collect_components(page, &mut component_map, &mut counter); @@ -280,7 +281,7 @@ pub fn figma_to_pen_document( }; let mut component_map: HashMap = HashMap::new(); - let mut symbol_tree: HashMap = HashMap::new(); + let mut symbol_tree: HashMap> = HashMap::new(); let mut counter: u32 = 1; collect_components(page, &mut component_map, &mut counter); collect_symbol_tree(&tree, &mut symbol_tree); @@ -376,7 +377,7 @@ pub fn figma_node_changes_to_pen_nodes( } let mut component_map: HashMap = HashMap::new(); - let mut symbol_tree: HashMap = HashMap::new(); + let mut symbol_tree: HashMap> = HashMap::new(); let mut counter: u32 = 1; for node in &top_nodes { collect_components(node, &mut component_map, &mut counter); diff --git a/crates/op-figma/src/tree.rs b/crates/op-figma/src/tree.rs index ff7d677d8..a520ba12e 100644 --- a/crates/op-figma/src/tree.rs +++ b/crates/op-figma/src/tree.rs @@ -5,6 +5,7 @@ use crate::figma_types::FigGuid; use crate::kiwi::FigValue; use std::collections::{HashMap, HashSet}; +use std::rc::Rc; /// Recursion ceiling for `materialize` — guards a malformed file with /// a cyclic parent chain. @@ -68,26 +69,34 @@ fn index_by_key(node_changes: &[FigValue]) -> (HashMap, Vec, ) -> (HashMap>, Option) { let mut children_of: HashMap> = HashMap::new(); let mut root_key: Option = None; - for nc in node_changes { - if nc.get_str("phase") == Some("REMOVED") { - continue; - } - let Some(key) = nc.get("guid").and_then(guid_to_string) else { + for key in order { + let Some(nc) = by_key.get(key) else { continue; }; - if !by_key.contains_key(&key) { - continue; - } if nc.get_str("type") == Some("DOCUMENT") { - root_key = Some(key); + root_key = Some(key.clone()); continue; } if let Some(parent_key) = nc @@ -96,7 +105,7 @@ fn build_adjacency( .and_then(guid_to_string) { if by_key.contains_key(&parent_key) { - children_of.entry(parent_key).or_default().push(key); + children_of.entry(parent_key).or_default().push(key.clone()); } } } @@ -105,13 +114,23 @@ fn build_adjacency( /// Recursively materialize a `TreeNode`; children sorted descending by /// `parentIndex.position` (z-stacking order — first child topmost). +/// +/// `by_key` is drained via `remove` rather than cloned: each guid is +/// normally visited exactly once (the adjacency map gives every node a +/// single parent slot), so ownership can move straight from the index +/// into the tree instead of paying for a second full-file clone. A +/// malformed file with a genuine parent cycle can revisit the same key +/// before `MAX_TREE_DEPTH` cuts the recursion off; a second visit then +/// sees an already-drained entry and falls back to `FigValue::Null` +/// (unchanged well-formed-file behavior; only the pathological-cycle +/// edge case differs from the old always-clone version). fn materialize( key: &str, - by_key: &HashMap, + by_key: &mut HashMap, children_of: &HashMap>, depth: u32, ) -> TreeNode { - let figma = by_key.get(key).cloned().unwrap_or(FigValue::Null); + let figma = by_key.remove(key).unwrap_or(FigValue::Null); let mut children = Vec::new(); if depth < MAX_TREE_DEPTH { if let Some(child_keys) = children_of.get(key) { @@ -131,28 +150,34 @@ fn materialize( /// Build the canonical document tree rooted at the `DOCUMENT` node. pub fn build_tree(node_changes: &[FigValue]) -> Option { - let (by_key, _order) = index_by_key(node_changes); - let (children_of, root_key) = build_adjacency(node_changes, &by_key); + let (mut by_key, order) = index_by_key(node_changes); + let (children_of, root_key) = build_adjacency(&order, &by_key); let root_key = root_key?; - Some(materialize(&root_key, &by_key, &children_of, 0)) + Some(materialize(&root_key, &mut by_key, &children_of, 0)) } /// Build orphan-rooted trees for clipboard data with no `DOCUMENT` /// wrapper — roots are nodes whose parent is absent. pub fn build_tree_for_clipboard(node_changes: &[FigValue]) -> Vec { - let (by_key, order) = index_by_key(node_changes); - let (children_of, _) = build_adjacency(node_changes, &by_key); + let (mut by_key, order) = index_by_key(node_changes); + let (children_of, _) = build_adjacency(&order, &by_key); let attached: HashSet<&String> = children_of.values().flatten().collect(); - order - .iter() + // Resolve the root-key filter (reads `by_key`) fully before + // `materialize` starts draining it, so the two phases don't need + // simultaneous conflicting borrows of the same map. + let root_keys: Vec = order + .into_iter() .filter(|k| !attached.contains(k)) .filter(|k| { by_key - .get(*k) + .get(k) .map(|n| n.get_str("type") != Some("DOCUMENT")) .unwrap_or(false) }) - .map(|k| materialize(k, &by_key, &children_of, 0)) + .collect(); + root_keys + .iter() + .map(|k| materialize(k, &mut by_key, &children_of, 0)) .collect() } @@ -172,10 +197,17 @@ pub fn collect_components(node: &TreeNode, map: &mut HashMap, co } /// Pre-order DFS: register every `SYMBOL` node's guid → its subtree. -pub fn collect_symbol_tree(node: &TreeNode, map: &mut HashMap) { +/// +/// The subtree is cloned once per distinct SYMBOL definition (the tree +/// walk that builds the page output still needs its own copy of the +/// master component), but wrapped in an `Rc` so every instance of that +/// SYMBOL shares the one clone — a cheap refcount bump per instance +/// instead of a second deep clone (previously the dominant cost on +/// instance-heavy files). +pub fn collect_symbol_tree(node: &TreeNode, map: &mut HashMap>) { if node.figma.get_str("type") == Some("SYMBOL") { if let Some(key) = node.figma.get("guid").and_then(guid_to_string) { - map.insert(key, node.clone()); + map.insert(key, Rc::new(node.clone())); } } for child in &node.children { @@ -264,6 +296,65 @@ mod tests { assert_eq!(roots.len(), 2); // FRAME + TEXT, RECTANGLE is nested } + #[test] + fn duplicate_guid_records_use_latest_parent_and_content_exactly_once() { + // Two node-change records share guid 4: an earlier one parented + // under FRAME 2 (position "x", name "v1") and a later one + // re-parented under FRAME 3 (position "y", name "v2"). Legal, + // non-cyclic `.fig` files can carry repeated guids like this + // (e.g. a node moved during editing) — `index_by_key` already + // picks the latest record's content; `build_adjacency` must + // agree and wire exactly one parent edge from that same latest + // record, instead of pushing the guid once per raw occurrence + // in `node_changes`. + let mut first = node(4, "RECTANGLE", Some(2), "x"); + if let FigValue::Object(pairs) = &mut first { + pairs.push(("name".into(), FigValue::Str("v1".into()))); + } + let mut second = node(4, "RECTANGLE", Some(3), "y"); + if let FigValue::Object(pairs) = &mut second { + pairs.push(("name".into(), FigValue::Str("v2".into()))); + } + let changes = vec![ + node(0, "DOCUMENT", None, ""), + node(1, "CANVAS", Some(0), "a"), + node(2, "FRAME", Some(1), "a"), + node(3, "FRAME", Some(1), "b"), + first, + second, + ]; + let tree = build_tree(&changes).expect("tree builds"); + let canvas = &tree.children[0]; + assert_eq!(canvas.children.len(), 2); + // Descending position sort ("b" > "a") puts guid-3's FRAME first. + let frame_b = &canvas.children[0]; + let frame_a = &canvas.children[1]; + + // Node 4 must appear under its LATEST parent (frame 3) exactly + // once, carrying the latest record's content ("v2") — never + // under the earlier parent (frame 2), and never duplicated. + assert!( + frame_a.children.is_empty(), + "node 4 must not remain under its stale parent" + ); + assert_eq!(frame_b.children.len(), 1, "node 4 must appear exactly once"); + assert_eq!(frame_b.children[0].figma.get_str("name"), Some("v2")); + + // No node in the materialized tree silently degraded to Null — + // the historical failure mode when a repeated guid revisited an + // already-`remove()`d `by_key` entry. + fn assert_no_null(n: &TreeNode) { + assert!( + !matches!(n.figma, FigValue::Null), + "unexpected Null node in tree" + ); + for c in &n.children { + assert_no_null(c); + } + } + assert_no_null(&tree); + } + #[test] fn removed_nodes_are_skipped() { let mut removed = node(2, "RECTANGLE", Some(1), "a");