crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md

@@ -171,26 +171,19 @@ compile error.
    wipe redundant. It previously did both on every sync, for all three
    viewports, on every frame of a drag.
 
-   **Part ids are shared, not copied.** All three viewports hold clones
-   of one `PartIdAllocator` (`Rc<Cell<u64>>`), wired up by
-   `CadWorkspace::share_part_id_allocator`. Allocate with
-   `part_ids.allocate()`; never keep a private counter.
+   Each viewport also owns a `next_part_id` allocator. A sync must
+   reconcile it with `scene_holder::reconcile_next_part_id`, never assign
+   it: only one viewport wins a frame, so the loser's allocations are
+   absent from the winner's snapshot and a direct assignment moves the
+   allocator *backwards*, reissuing live ids.
 
-   This was three independent `u64`s synced by copy. Only one viewport
-   wins a sync frame, so the loser's allocations were absent from the
-   winner's snapshot and the assignment moved its counter *backwards* —
-   two parts drawn in different views between syncs got the same
-   `NodeId`, which keys `MeshCache`, `part_geoms` and every command.
-   Reconciling on each sync bounded that; sharing removes it.
-
-   Parts adopted from outside the allocator — script rebuild, file load,
-   undo restore — carry ids it never issued, so `reserve_past_nodes` runs
-   on every `replace_parts_snapshot`.
-
-   The parts list itself is still copied. That is the rest of Phase 5.2:
-   one `Rc<RefCell<CadDocument>>` and no snapshot pair. The id allocator
-   is the proof of the approach — one counter duplicated three ways could
-   only ever be bounded, never made correct.
+   This is still a copy, not shared ownership, and the id allocator is
+   the clearest evidence that copying is the wrong model — one counter
+   duplicated three ways cannot be made correct, only bounded. The
+   remaining Phase 5.2 work is to hand all three viewports one
+   `Rc<RefCell<CadDocument>>` and delete the snapshot pair; until then
+   the generation check bounds the cost and the reconcile bounds the
+   damage.
 
 5. **`CadTransform::rotation_euler_xyz` is in DEGREES.** This is the
    crate-wide contract: `math::rot_*_mat`, `makepad_csg::Solid::rotate_*`,

crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs

@@ -89,7 +89,7 @@ pub use cad_scene::{
     NodeMetadata, PartKind, SceneBuilder, SceneMeta, SceneUnits, SceneVisitor,
     SheetId, walk_scene,
 };
-pub use scene_holder::{PartIdAllocator, PartsStore, SceneCache};
+pub use scene_holder::{PartsStore, SceneCache};
 // Re-exports so call sites need not name the sub-module.
 pub(crate) use constants::{DEFAULT_CAD_SCRIPT, LIVE_UPDATE_INTERVAL, local_openai_url, local_openai_model, GENERATED_DIR, GENERATED_SCRIPT_FILE, GENERATED_OBJ_FILE, DEMO_MAX_CURVE_SEGMENTS, DEMO_MAX_SPHERE_RINGS, DEMO_MAX_TORUS_MINOR_SEGMENTS, PART_SELECT_COLOR, PART_PICK_RADIUS, MAX_UNDO_LEVELS, HOVER_PICK_MIN_MOVE_PX, CAD_SCRIPT_TIME_BUDGET, CAD_SCRIPT_BUDGET_SAMPLE_INSTRUCTIONS};
 pub(crate) use persistence::{PENDING_ATTACHED_IMAGE, CAD_SCRIPT_OUTPUT, cad_manifest_path, cad_generated_dir_path, cad_generated_script_path, cad_generated_obj_path, load_saved_cad_script, save_cad_state, set_cad_script_output, clear_cad_script_output, take_cad_script_output};
@@ -1659,11 +1659,8 @@ pub struct CadViewport {
     /// `mark_dirty()` + `invalidate_node(id)`.
     #[rust]
     scene_cache: SceneCache,
-    /// Shared with the other two viewports (see
-    /// `CadWorkspace::share_part_id_allocator`). Not a plain `u64`:
-    /// three independent counters synced by copy reissued live ids.
-    #[rust]
-    part_ids: PartIdAllocator,
+    #[rust(1u64)]
+    next_part_id: u64,
     #[rust]
     selection: Vec<u64>,
     #[rust(ViewMode::ThreeD)]

crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/scene_holder.rs

@@ -464,84 +464,35 @@ mod tests {
 /// This is the first increment of Phase 5.1. `CadViewport` still owns the
 /// store directly; sharing one document across the three viewports (5.2)
 /// is a separate change.
-/// Part-id allocator shared by every `CadViewport`.
+/// Reconcile a destination viewport's id allocator with an incoming
+/// snapshot.
 ///
-/// The three viewports used to hold one `u64` each, kept in step by
-/// copying whole snapshots. That cannot be made correct: only one
-/// viewport wins a sync frame, so the loser's allocations are absent
-/// from the winner's snapshot and a direct assignment moves the
-/// allocator backwards, reissuing ids that are still live. Two parts
-/// drawn in different views between two syncs got the same `NodeId` --
-/// which keys `MeshCache`, `part_geoms` and every command.
+/// The three `CadViewport`s each own a `next_part_id` counter and are
+/// kept in step by copying whole snapshots between them. Assigning the
+/// source's counter directly -- which is what the sync used to do --
+/// can move a destination's counter *backwards*, because only one
+/// viewport wins per frame and the loser's allocations are not in the
+/// winner's snapshot.
 ///
-/// Reconciling the counters on every sync bounded that damage. Sharing
-/// one counter removes it: an id handed out anywhere is never handed
-/// out again, with no sync step involved.
+/// Two parts drawn in different viewports between two syncs therefore
+/// both got the same id, and the loser's part was silently dropped.
 ///
-/// `Rc<Cell<u64>>` rather than `Arc`: the viewports live on the UI
-/// thread. `Cell` because a `u64` is `Copy` and no borrow can outlive
-/// the call, so there is no `RefCell` panic to reason about.
-///
-/// This is the first piece of Phase 5.2 to become genuinely shared
-/// rather than copied. The parts list itself is next.
-#[derive(Clone, Debug)]
-pub struct PartIdAllocator {
-    next: std::rc::Rc<std::cell::Cell<u64>>,
-}
-
-impl PartIdAllocator {
-    /// A fresh allocator starting at `first`.
-    pub fn new(first: u64) -> Self {
-        Self { next: std::rc::Rc::new(std::cell::Cell::new(first)) }
-    }
-
-    /// Hand out the next id.
-    ///
-    /// Saturates rather than wrapping: reissuing id 0 after 2^64 parts
-    /// would alias a live node, which is worse than refusing to
-    /// allocate.
-    pub fn allocate(&self) -> u64 {
-        let id = self.next.get();
-        self.next.set(id.saturating_add(1));
-        id
-    }
-
-    /// The id that `allocate` will return next.
-    pub fn peek(&self) -> u64 {
-        self.next.get()
-    }
-
-    /// Ensure no id below `bound` is ever handed out.
-    ///
-    /// Used when adopting parts from outside the allocator -- a script
-    /// rebuild, a file load, an undo restore -- whose ids were not
-    /// issued by it. Never lowers the counter.
-    pub fn reserve_up_to(&self, bound: u64) {
-        if bound > self.next.get() {
-            self.next.set(bound);
-        }
-    }
-
-    /// Reserve past every id present in `nodes`.
-    pub fn reserve_past_nodes(&self, nodes: &[CadNode]) {
-        if let Some(max) = nodes.iter().map(|n| n.id.raw()).max() {
-            self.reserve_up_to(max.saturating_add(1));
-        }
-    }
-
-    /// True when both handles refer to the same counter.
-    ///
-    /// Only meaningful in tests: production code shares one allocator by
-    /// construction, and this is what proves it.
-    pub fn shares_with(&self, other: &Self) -> bool {
-        std::rc::Rc::ptr_eq(&self.next, &other.next)
-    }
-}
-
-impl Default for PartIdAllocator {
-    fn default() -> Self {
-        Self::new(1)
-    }
+/// The counter must never retreat, and must never collide with an id
+/// already present in the incoming parts, so take the maximum of:
+/// what the destination has already handed out, what the source has
+/// handed out, and one past the highest id actually in the list.
+pub fn reconcile_next_part_id(
+    destination_next: u64,
+    source_next: u64,
+    incoming: &[CadNode],
+) -> u64 {
+    let highest_in_use = incoming
+        .iter()
+        .map(|n| n.id.raw())
+        .max()
+        .map(|m| m.saturating_add(1))
+        .unwrap_or(0);
+    destination_next.max(source_next).max(highest_in_use)
 }
 
 #[derive(Debug, Default)]
@@ -822,65 +773,51 @@ mod scene_cache_generation_tests {
         );
     }
 
-    // ---- shared id allocator ----
+    // ---- id allocator reconciliation ----
 
-    /// The defect that motivated sharing: two viewports allocating
-    /// between syncs used to produce the same id. A shared allocator
-    /// cannot, because there is one counter.
+    /// The sync used to assign the source's `next_part_id` directly,
+    /// which can move a destination's allocator BACKWARDS.
+    ///
+    /// Only one viewport wins per sync frame. If the user draws in the
+    /// 2D view and the 3D view between two syncs, both allocate the
+    /// same id independently, and the loser's counter is then reset to
+    /// the winner's -- so the very next part reuses an id that is
+    /// already live.
     #[test]
-    fn two_handles_never_hand_out_the_same_id() {
-        let view_2d = PartIdAllocator::new(1);
-        let view_3d = view_2d.clone();
-
-        let a = view_2d.allocate();
-        let b = view_3d.allocate();
-        let c = view_2d.allocate();
-
-        assert_eq!((a, b, c), (1, 2, 3), "ids must be globally unique");
-        assert!(
-            view_2d.shares_with(&view_3d),
-            "clone must alias the counter, not copy it"
+    fn reconciling_never_moves_the_allocator_backwards() {
+        // Destination has handed out up to 41; source only up to 20.
+        let next = reconcile_next_part_id(42, 20, &[]);
+        assert_eq!(
+            next, 42,
+            "the destination's allocator must not retreat to the source's"
         );
-        assert_eq!(view_3d.peek(), 4, "both handles observe every allocation");
     }
 
-    /// A separately constructed allocator must NOT share -- otherwise
-    /// the test above would pass for the wrong reason.
+    /// It must also clear every id actually present in the incoming
+    /// list, even when both counters are stale -- a snapshot restored
+    /// from disk or from undo can carry ids above both.
     #[test]
-    fn separately_constructed_allocators_do_not_share() {
-        let a = PartIdAllocator::new(1);
-        let b = PartIdAllocator::new(1);
-        assert!(!a.shares_with(&b));
-        a.allocate();
-        assert_eq!(b.peek(), 1, "an unrelated allocator is unaffected");
+    fn reconciling_clears_the_highest_incoming_id() {
+        let parts = vec![node(3), node(99), node(7)];
+        let next = reconcile_next_part_id(5, 5, &parts);
+        assert_eq!(
+            next, 100,
+            "an id already in the list must never be handed out again"
+        );
     }
 
-    /// Parts arriving from outside the allocator -- a script rebuild, a
-    /// file load, an undo restore -- carry ids it never issued. It must
-    /// step past them or the next allocation aliases a live node.
+    /// The ordinary case: nothing stale, so the counters agree.
     #[test]
-    fn reserving_past_adopted_nodes_prevents_reuse() {
-        let alloc = PartIdAllocator::new(1);
-        alloc.reserve_past_nodes(&[node(3), node(99), node(7)]);
-        assert_eq!(alloc.allocate(), 100);
+    fn reconciling_is_a_no_op_when_the_viewports_agree() {
+        let parts = vec![node(1), node(2)];
+        assert_eq!(reconcile_next_part_id(3, 3, &parts), 3);
     }
 
-    /// Reserving must never lower the counter: ids already handed out
-    /// are live even if the incoming list happens to be shorter.
+    /// u64::MAX must not wrap into 0 and start reissuing live ids.
     #[test]
-    fn reserving_never_moves_the_counter_backwards() {
-        let alloc = PartIdAllocator::new(50);
-        alloc.reserve_up_to(10);
-        alloc.reserve_past_nodes(&[node(2)]);
-        assert_eq!(alloc.peek(), 50);
-    }
-
-    /// Saturate rather than wrap. Wrapping to 0 would reissue live ids.
-    #[test]
-    fn allocation_saturates_at_the_top_of_the_range() {
-        let alloc = PartIdAllocator::new(u64::MAX);
-        assert_eq!(alloc.allocate(), u64::MAX);
-        assert_eq!(alloc.peek(), u64::MAX, "must not wrap to 0");
+    fn reconciling_saturates_instead_of_wrapping() {
+        let parts = vec![node(u64::MAX)];
+        assert_eq!(reconcile_next_part_id(0, 0, &parts), u64::MAX);
     }
 
     #[test]

crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs

@@ -131,24 +131,12 @@ impl CadViewport {
         cx.redraw_all();
     }
 
-    pub(crate) fn parts_snapshot(&self) -> (Vec<CadNode>, Vec<u64>) {
-        (self.parts.as_slice().to_vec(), self.selection.clone())
-    }
-
-    /// Hand this viewport the allocator the others use.
-    ///
-    /// Called once per viewport at startup. Until every viewport shares
-    /// one, ids are only unique per view.
-    pub(crate) fn adopt_part_id_allocator(&mut self, shared: PartIdAllocator) {
-        // Anything this viewport already handed out must stay reserved.
-        shared.reserve_up_to(self.part_ids.peek());
-        shared.reserve_past_nodes(self.parts.as_slice());
-        self.part_ids = shared;
-    }
-
-    /// The allocator handle, for sharing with the other viewports.
-    pub(crate) fn part_id_allocator(&self) -> PartIdAllocator {
-        self.part_ids.clone()
+    pub(crate) fn parts_snapshot(&self) -> (Vec<CadNode>, Vec<u64>, u64) {
+        (
+            self.parts.as_slice().to_vec(),
+            self.selection.clone(),
+            self.next_part_id,
+        )
     }
 
     /// The `PartsStore` generation this viewport is currently showing.
@@ -166,6 +154,7 @@ impl CadViewport {
         cx: &mut Cx,
         parts: Vec<CadNode>,
         selection: Vec<u64>,
+        next_part_id: u64,
     ) {
         // Which node ids survive this replacement. GPU geometry for a
         // surviving id is still valid -- the mesh is rebuilt from the
@@ -180,11 +169,15 @@ impl CadViewport {
             parts.iter().map(|p| p.id.raw()).collect();
         self.part_geoms.retain(|id, _| incoming.contains(id));
 
-        // No id reconciliation here any more: the allocator is shared,
-        // so an id handed out in any viewport is already accounted for
-        // in all of them. Parts adopted from outside the allocator
-        // (script rebuild, file load) still need reserving.
-        self.part_ids.reserve_past_nodes(&parts);
+        // Never let the id allocator retreat. Only one viewport wins a
+        // sync frame, so the loser's allocations are absent from this
+        // snapshot -- assigning the source's counter directly would
+        // reissue ids that are already live in the other view.
+        self.next_part_id = super::scene_holder::reconcile_next_part_id(
+            self.next_part_id,
+            next_part_id,
+            &parts,
+        );
         self.parts.replace_all(parts);
         self.selection = selection;
         // The scene snapshot is rebuilt on demand: `scene_for` compares
@@ -461,7 +454,8 @@ impl CadViewport {
     }
 
     pub(crate) fn add_part(&mut self, cx: &mut Cx, kind: PartKind) -> u64 {
-        let id = self.part_ids.allocate();
+        let id = self.next_part_id;
+        self.next_part_id += 1;
         let (size, color) = match kind {
             PartKind::Cube => (vec3(1.0, 1.0, 1.0), vec4(0.34, 0.74, 0.86, 1.0)),
             PartKind::Cylinder => (vec3(1.0, 1.2, 1.0), vec4(0.55, 0.82, 0.55, 1.0)),
@@ -1223,7 +1217,8 @@ impl CadViewport {
         let seg_len = total_len / n as f32;
         let mut new_ids = Vec::new();
         for i in 0..n {
-            let new_id = self.part_ids.allocate();
+            let new_id = self.next_part_id;
+            self.next_part_id += 1;
             let offset = (i as f32 + 0.5) * seg_len - total_len * 0.5;
             let mut pos = part.pos();
             let mut size = part.size();
@@ -1336,7 +1331,8 @@ impl CadViewport {
         let clipboard = self.clipboard_parts.clone();
         let mut new_ids = Vec::new();
         for part in &clipboard {
-            let id = self.part_ids.allocate();
+            let id = self.next_part_id;
+            self.next_part_id += 1;
             let mut new_part = part.clone();
             new_part.id = NodeId(id);
             let mut p = new_part.pos();
@@ -3267,7 +3263,8 @@ impl CadViewport {
         }
         let start = self.drawing.start_world;
         let end = self.drawing.current_world;
-        let id = self.part_ids.allocate();
+        let id = self.next_part_id;
+        self.next_part_id += 1;
         match self.drawing.tool {
             CadTool::Arc => {
                 if let Some(mid) = self.drawing.arc_mid_world {
@@ -3706,7 +3703,8 @@ impl CadViewport {
             } else {
                 (0.2f32, dist as f32)
             };
-            let id = self.part_ids.allocate();
+            let id = self.next_part_id;
+            self.next_part_id += 1;
             self.parts.push(CadNode {
                 id: NodeId(id),
                 name: format!("Wall-{}", id),

crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs

@@ -314,20 +314,20 @@ impl CadWorkspace {
             .widget(cx, ids!(cad_viewport))
             .borrow_mut::<CadViewport>()
         {
-            let (parts, selected) = vp.parts_snapshot();
+            let (parts, selected, next_id) = vp.parts_snapshot();
             if let Some(mut vp2d) = self
                 .view
                 .widget(cx, ids!(cad_viewport_2d))
                 .borrow_mut::<CadViewport>()
             {
-                vp2d.replace_parts_snapshot(cx, parts.clone(), selected.clone());
+                vp2d.replace_parts_snapshot(cx, parts.clone(), selected.clone(), next_id);
             }
             if let Some(mut vp3d) = self
                 .view
                 .widget(cx, ids!(cad_viewport_3d))
                 .borrow_mut::<CadViewport>()
             {
-                vp3d.replace_parts_snapshot(cx, parts, selected);
+                vp3d.replace_parts_snapshot(cx, parts, selected, next_id);
             }
         }
     }
@@ -617,8 +617,8 @@ impl CadWorkspace {
                     return None;
                 }
                 let script = vp.generate_parts_script();
-                let (parts, selected) = vp.parts_snapshot();
-                Some((script, parts, selected, vp.parts_generation()))
+                let (parts, selected, next_id) = vp.parts_snapshot();
+                Some((script, parts, selected, next_id, vp.parts_generation()))
             });
             if let Some(Some(taken)) = taken {
                 if snapshot.is_none() {
@@ -627,7 +627,7 @@ impl CadWorkspace {
                 }
             }
         }
-        let (Some((script, parts, selected, source_generation)), Some(source)) =
+        let (Some((script, parts, selected, next_id, source_generation)), Some(source)) =
             (snapshot, source)
         else {
             return None;
@@ -651,7 +651,7 @@ impl CadWorkspace {
                 continue;
             }
             self.with_viewport(cx, which, |vp, cx| {
-                vp.replace_parts_snapshot(cx, parts.clone(), selected.clone());
+                vp.replace_parts_snapshot(cx, parts.clone(), selected.clone(), next_id);
             });
         }
         if script.is_empty() {
@@ -803,27 +803,7 @@ impl CadWorkspace {
         self.set_viewport_layout_mode(cx, next);
     }
 
-    /// Give all three viewports the same `PartIdAllocator`.
-    ///
-    /// Idempotent and cheap, so it runs on the same path as worker
-    /// creation rather than needing its own init hook. Until this has
-    /// run, each viewport allocates from its own counter and ids are
-    /// only unique per view -- which is the bug this exists to remove.
-    fn share_part_id_allocator(&mut self, cx: &mut Cx) {
-        let Some(shared) = self.with_viewport(cx, 0, |vp, _cx| vp.part_id_allocator()) else {
-            return;
-        };
-        for which in [1usize, 2] {
-            self.with_viewport(cx, which, |vp, _cx| {
-                if !vp.part_id_allocator().shares_with(&shared) {
-                    vp.adopt_part_id_allocator(shared.clone());
-                }
-            });
-        }
-    }
-
     fn request_rebuild(&mut self, cx: &mut Cx, force: bool, save_output: bool) {
-        self.share_part_id_allocator(cx);
         if self.cad_worker.is_none() {
             self.cad_worker = Some(CadRebuildWorker::new(cx));
         }

crates/apps/nigig-build/src/construction_frame/pages/workspace/project/mod.rs

@@ -813,17 +813,18 @@ impl Widget for BuildProjectsPage {
                 if let Some(mut ws) = self.view.widget(cx, ids!(spreadsheet_workspace)).borrow_mut::<spreadsheet_ui::SpreadsheetWorkspace>() {
                     ws.save_to_disk(cx);
                 }
-                // 2) Point the mobile grid at the saved workbook.
-                //
-                // The grid no longer owns a SpreadsheetData; it reads
-                // through a SharedWorkspaceModel. `load_saved` handles
-                // both the multi-sheet and the legacy single-sheet
-                // formats, which this used to open-code.
+                // 2) Load the active sheet's data into the mobile grid
                 if let Some(mut grid) = self.view.widget(cx, ids!(m_ss_grid)).borrow_mut::<spreadsheet_ui::SpreadsheetGrid>() {
-                    if let Some(model) = spreadsheet_ui::model::WorkspaceModel::load_saved() {
-                        grid.attach_workspace_model(std::rc::Rc::new(
-                            std::cell::RefCell::new(model),
-                        ));
+                    if let Some(saved) = spreadsheet_ui::persistence::load_saved_spreadsheet_state() {
+                        if let Some((sheets, active_idx)) = spreadsheet_ui::workbook::deserialize_workbook(&saved) {
+                            if let Some(sheet) = sheets.get(active_idx) {
+                                grid.data = sheet.data.clone();
+                            }
+                        } else {
+                            // Legacy single-sheet format
+                            grid.data = spreadsheet_ui::SpreadsheetData::default();
+                            grid.data.deserialize(&saved);
+                        }
                     }
                     grid.selection_anchor = Some((0, 0));
                     grid.selection_head = Some((0, 0));
@@ -835,13 +836,19 @@ impl Widget for BuildProjectsPage {
                 // Was mobile → going to desktop:
                 // 1) Update the saved workbook with mobile grid's active sheet data
                 //    (preserves the other sheets that were saved from desktop)
-                // The grid edits the shared model in place, so saving it
-                // preserves every sheet -- no read-modify-write of the
-                // active sheet against the on-disk workbook.
                 if let Some(grid) = self.view.widget(cx, ids!(m_ss_grid)).borrow::<spreadsheet_ui::SpreadsheetGrid>() {
-                    if let Some(model) = &grid.shared_model {
-                        if let Err(e) = model.borrow().save() {
-                            error!("spreadsheet: mobile->desktop save failed: {e}");
+                    let active_data = grid.data.serialize();
+                    if let Some(saved) = spreadsheet_ui::persistence::load_saved_spreadsheet_state() {
+                        if let Some((mut sheets, active_idx)) = spreadsheet_ui::workbook::deserialize_workbook(&saved) {
+                            if let Some(sheet) = sheets.get_mut(active_idx) {
+                                sheet.data = spreadsheet_ui::SpreadsheetData::default();
+                                sheet.data.deserialize(&active_data);
+                            }
+                            let updated = spreadsheet_ui::workbook::serialize_workbook(&sheets, active_idx);
+                            let _ = spreadsheet_ui::persistence::save_spreadsheet_state(&updated);
+                        } else {
+                            // Legacy format — just save the grid data as-is
+                            let _ = spreadsheet_ui::persistence::save_spreadsheet_state(&active_data);
                         }
                     }
                 }