Prevent video and audio clips from overlapping

lightningbeam-ui/lightningbeam-core/src/action.rs

@@ -58,10 +58,14 @@ pub struct BackendContext<'a> {
 /// only affect the document (vector graphics) don't need to implement these.
 pub trait Action: Send {
     /// Apply this action to the document
-    fn execute(&mut self, document: &mut Document);
+    ///
+    /// Returns Ok(()) if successful, or Err(message) if the action cannot be performed
+    fn execute(&mut self, document: &mut Document) -> Result<(), String>;
 
     /// Undo this action (rollback changes)
-    fn rollback(&mut self, document: &mut Document);
+    ///
+    /// Returns Ok(()) if successful, or Err(message) if rollback fails
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String>;
 
     /// Get a human-readable description of this action (for UI display)
     fn description(&self) -> String;
@@ -158,9 +162,11 @@ impl ActionExecutor {
     /// Execute an action and add it to the undo stack
     ///
     /// This clears the redo stack since we're creating a new timeline branch.
-    pub fn execute(&mut self, mut action: Box<dyn Action>) {
+    ///
+    /// Returns Ok(()) if successful, or Err(message) if the action failed
+    pub fn execute(&mut self, mut action: Box<dyn Action>) -> Result<(), String> {
         // Apply the action (uses copy-on-write if other Arc holders exist)
-        action.execute(Arc::make_mut(&mut self.document));
+        action.execute(Arc::make_mut(&mut self.document))?;
 
         // Clear redo stack (new action invalidates redo history)
         self.redo_stack.clear();
@@ -172,39 +178,55 @@ impl ActionExecutor {
         if self.undo_stack.len() > self.max_undo_depth {
             self.undo_stack.remove(0);
         }
+
+        Ok(())
     }
 
     /// Undo the last action
     ///
-    /// Returns true if an action was undone, false if undo stack is empty.
+    /// Returns Ok(true) if an action was undone, Ok(false) if undo stack is empty,
-    pub fn undo(&mut self) -> bool {
+    /// or Err(message) if rollback failed
+    pub fn undo(&mut self) -> Result<bool, String> {
         if let Some(mut action) = self.undo_stack.pop() {
             // Rollback the action (uses copy-on-write if other Arc holders exist)
-            action.rollback(Arc::make_mut(&mut self.document));
+            match action.rollback(Arc::make_mut(&mut self.document)) {
-
+                Ok(()) => {
                     // Move to redo stack
                     self.redo_stack.push(action);
-
+                    Ok(true)
-            true
+                }
+                Err(e) => {
+                    // Put action back on undo stack if rollback failed
+                    self.undo_stack.push(action);
+                    Err(e)
+                }
+            }
         } else {
-            false
+            Ok(false)
         }
     }
 
     /// Redo the last undone action
     ///
-    /// Returns true if an action was redone, false if redo stack is empty.
+    /// Returns Ok(true) if an action was redone, Ok(false) if redo stack is empty,
-    pub fn redo(&mut self) -> bool {
+    /// or Err(message) if re-execution failed
+    pub fn redo(&mut self) -> Result<bool, String> {
         if let Some(mut action) = self.redo_stack.pop() {
             // Re-execute the action (uses copy-on-write if other Arc holders exist)
-            action.execute(Arc::make_mut(&mut self.document));
+            match action.execute(Arc::make_mut(&mut self.document)) {
-
+                Ok(()) => {
                     // Move back to undo stack
                     self.undo_stack.push(action);
-
+                    Ok(true)
-            true
+                }
+                Err(e) => {
+                    // Put action back on redo stack if re-execution failed
+                    self.redo_stack.push(action);
+                    Err(e)
+                }
+            }
         } else {
-            false
+            Ok(false)
         }
     }
 
@@ -273,12 +295,12 @@ impl ActionExecutor {
         backend: &mut BackendContext,
     ) -> Result<(), String> {
         // 1. Execute document changes
-        action.execute(Arc::make_mut(&mut self.document));
+        action.execute(Arc::make_mut(&mut self.document))?;
 
         // 2. Execute backend changes (pass document for reading clip data)
         if let Err(e) = action.execute_backend(backend, &self.document) {
             // ATOMIC ROLLBACK: Backend failed → undo document
-            action.rollback(Arc::make_mut(&mut self.document));
+            action.rollback(Arc::make_mut(&mut self.document))?;
             return Err(e);
         }
 
@@ -309,7 +331,7 @@ impl ActionExecutor {
         if let Some(mut action) = self.undo_stack.pop() {
             // Rollback in REVERSE order: backend first, then document
             action.rollback_backend(backend, &self.document)?;
-            action.rollback(Arc::make_mut(&mut self.document));
+            action.rollback(Arc::make_mut(&mut self.document))?;
 
             // Move to redo stack
             self.redo_stack.push(action);
@@ -334,11 +356,15 @@ impl ActionExecutor {
     pub fn redo_with_backend(&mut self, backend: &mut BackendContext) -> Result<bool, String> {
         if let Some(mut action) = self.redo_stack.pop() {
             // Re-execute in same order: document first, then backend
-            action.execute(Arc::make_mut(&mut self.document));
+            if let Err(e) = action.execute(Arc::make_mut(&mut self.document)) {
+                // Put action back on redo stack if document execute fails
+                self.redo_stack.push(action);
+                return Err(e);
+            }
 
             if let Err(e) = action.execute_backend(backend, &self.document) {
                 // Rollback document if backend fails
-                action.rollback(Arc::make_mut(&mut self.document));
+                action.rollback(Arc::make_mut(&mut self.document))?;
                 // Put action back on redo stack
                 self.redo_stack.push(action);
                 return Err(e);
@@ -374,12 +400,14 @@ mod tests {
     }
 
     impl Action for TestAction {
-        fn execute(&mut self, _document: &mut Document) {
+        fn execute(&mut self, _document: &mut Document) -> Result<(), String> {
             self.executed = true;
+            Ok(())
         }
 
-        fn rollback(&mut self, _document: &mut Document) {
+        fn rollback(&mut self, _document: &mut Document) -> Result<(), String> {
             self.executed = false;
+            Ok(())
         }
 
         fn description(&self) -> String {
@@ -397,20 +425,20 @@ mod tests {
 
         // Execute an action
         let action = Box::new(TestAction::new("Test Action"));
-        executor.execute(action);
+        executor.execute(action).unwrap();
 
         assert!(executor.can_undo());
         assert!(!executor.can_redo());
         assert_eq!(executor.undo_depth(), 1);
 
         // Undo
-        assert!(executor.undo());
+        assert!(executor.undo().unwrap());
         assert!(!executor.can_undo());
         assert!(executor.can_redo());
         assert_eq!(executor.redo_depth(), 1);
 
         // Redo
-        assert!(executor.redo());
+        assert!(executor.redo().unwrap());
         assert!(executor.can_undo());
         assert!(!executor.can_redo());
     }
@@ -420,12 +448,12 @@ mod tests {
         let document = Document::new("Test");
         let mut executor = ActionExecutor::new(document);
 
-        executor.execute(Box::new(TestAction::new("Action 1")));
+        executor.execute(Box::new(TestAction::new("Action 1"))).unwrap();
-        executor.execute(Box::new(TestAction::new("Action 2")));
+        executor.execute(Box::new(TestAction::new("Action 2"))).unwrap();
 
         assert_eq!(executor.undo_description(), Some("Action 2".to_string()));
 
-        executor.undo();
+        executor.undo().unwrap();
         assert_eq!(executor.redo_description(), Some("Action 2".to_string()));
         assert_eq!(executor.undo_description(), Some("Action 1".to_string()));
     }
@@ -435,14 +463,14 @@ mod tests {
         let document = Document::new("Test");
         let mut executor = ActionExecutor::new(document);
 
-        executor.execute(Box::new(TestAction::new("Action 1")));
+        executor.execute(Box::new(TestAction::new("Action 1"))).unwrap();
-        executor.execute(Box::new(TestAction::new("Action 2")));
+        executor.execute(Box::new(TestAction::new("Action 2"))).unwrap();
-        executor.undo();
+        executor.undo().unwrap();
 
         assert!(executor.can_redo());
 
         // Execute new action should clear redo stack
-        executor.execute(Box::new(TestAction::new("Action 3")));
+        executor.execute(Box::new(TestAction::new("Action 3"))).unwrap();
 
         assert!(!executor.can_redo());
         assert_eq!(executor.undo_depth(), 2);
@@ -454,10 +482,10 @@ mod tests {
         let mut executor = ActionExecutor::new(document);
         executor.set_max_undo_depth(3);
 
-        executor.execute(Box::new(TestAction::new("Action 1")));
+        executor.execute(Box::new(TestAction::new("Action 1"))).unwrap();
-        executor.execute(Box::new(TestAction::new("Action 2")));
+        executor.execute(Box::new(TestAction::new("Action 2"))).unwrap();
-        executor.execute(Box::new(TestAction::new("Action 3")));
+        executor.execute(Box::new(TestAction::new("Action 3"))).unwrap();
-        executor.execute(Box::new(TestAction::new("Action 4")));
+        executor.execute(Box::new(TestAction::new("Action 4"))).unwrap();
 
         // Should only keep last 3
         assert_eq!(executor.undo_depth(), 3);

lightningbeam-ui/lightningbeam-core/src/actions/add_clip_instance.rs

@@ -59,8 +59,35 @@ impl AddClipInstanceAction {
 }
 
 impl Action for AddClipInstanceAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
-        if let Some(layer) = document.get_layer_mut(&self.layer_id) {
+        // Calculate the clip's effective duration
+        let clip_duration = document.get_clip_duration(&self.clip_instance.clip_id)
+            .ok_or_else(|| format!("Clip {} not found", self.clip_instance.clip_id))?;
+
+        let trim_start = self.clip_instance.trim_start;
+        let trim_end = self.clip_instance.trim_end.unwrap_or(clip_duration);
+        let effective_duration = trim_end - trim_start;
+
+        // Auto-adjust position for audio/video layers to avoid overlaps
+        let adjusted_start = document.find_nearest_valid_position(
+            &self.layer_id,
+            self.clip_instance.timeline_start,
+            effective_duration,
+            None, // Not excluding any instance
+        );
+
+        if let Some(valid_start) = adjusted_start {
+            // Update instance to use the valid position
+            self.clip_instance.timeline_start = valid_start;
+        } else {
+            // No valid position found - reject the operation
+            return Err("Cannot add clip: no valid position found on layer (layer is full)".to_string());
+        }
+
+        // Add the clip instance with adjusted position
+        let layer = document.get_layer_mut(&self.layer_id)
+            .ok_or_else(|| format!("Layer {} not found", self.layer_id))?;
+
         match layer {
             AnyLayer::Vector(vector_layer) => {
                 vector_layer.clip_instances.push(self.clip_instance.clone());
@@ -73,17 +100,20 @@ impl Action for AddClipInstanceAction {
             }
         }
         self.executed = true;
-        }
+
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         if !self.executed {
-            return;
+            return Ok(());
         }
 
         let instance_id = self.clip_instance.id;
 
-        if let Some(layer) = document.get_layer_mut(&self.layer_id) {
+        let layer = document.get_layer_mut(&self.layer_id)
+            .ok_or_else(|| format!("Layer {} not found", self.layer_id))?;
+
         match layer {
             AnyLayer::Vector(vector_layer) => {
                 vector_layer
@@ -102,7 +132,8 @@ impl Action for AddClipInstanceAction {
             }
         }
         self.executed = false;
-        }
+
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -261,7 +292,7 @@ mod tests {
 
         // Execute action
         let mut action = AddClipInstanceAction::new(layer_id, clip_instance);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify clip instance was added
         if let Some(AnyLayer::Vector(vector_layer)) = document.get_layer(&layer_id) {
@@ -272,7 +303,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify clip instance was removed
         if let Some(AnyLayer::Vector(vector_layer)) = document.get_layer(&layer_id) {

lightningbeam-ui/lightningbeam-core/src/actions/add_layer.rs

@@ -49,15 +49,17 @@ impl AddLayerAction {
 }
 
 impl Action for AddLayerAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         // Add layer to the document's root
         let layer_id = document.root_mut().add_child(self.layer.clone());
 
         // Store the ID for rollback
         self.created_layer_id = Some(layer_id);
+
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         // Remove the created layer if it exists
         if let Some(layer_id) = self.created_layer_id {
             document.root_mut().remove_child(&layer_id);
@@ -65,6 +67,8 @@ impl Action for AddLayerAction {
             // Clear the stored ID
             self.created_layer_id = None;
         }
+
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -88,7 +92,7 @@ mod tests {
 
         // Create and execute action
         let mut action = AddLayerAction::new_vector("New Layer");
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify layer was added
         assert_eq!(document.root.children.len(), 1);
@@ -97,7 +101,7 @@ mod tests {
         assert!(matches!(layer, AnyLayer::Vector(_)));
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify layer was removed
         assert_eq!(document.root.children.len(), 0);
@@ -116,8 +120,8 @@ mod tests {
         let mut action1 = AddLayerAction::new_vector("Layer 1");
         let mut action2 = AddLayerAction::new_vector("Layer 2");
 
-        action1.execute(&mut document);
+        action1.execute(&mut document).unwrap();
-        action2.execute(&mut document);
+        action2.execute(&mut document).unwrap();
 
         assert_eq!(document.root.children.len(), 2);
         assert_eq!(document.root.children[0].layer().name, "Layer 1");

lightningbeam-ui/lightningbeam-core/src/actions/add_shape.rs

@@ -50,10 +50,10 @@ impl AddShapeAction {
 }
 
 impl Action for AddShapeAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         let layer = match document.get_layer_mut(&self.layer_id) {
             Some(l) => l,
-            None => return,
+            None => return Ok(()),
         };
 
         if let AnyLayer::Vector(vector_layer) = layer {
@@ -65,14 +65,15 @@ impl Action for AddShapeAction {
             self.created_shape_id = Some(shape_id);
             self.created_object_id = Some(object_id);
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         // Remove the created shape and object if they exist
         if let (Some(shape_id), Some(object_id)) = (self.created_shape_id, self.created_object_id) {
             let layer = match document.get_layer_mut(&self.layer_id) {
                 Some(l) => l,
-                None => return,
+                None => return Ok(()),
             };
 
             if let AnyLayer::Vector(vector_layer) = layer {
@@ -85,6 +86,7 @@ impl Action for AddShapeAction {
             self.created_shape_id = None;
             self.created_object_id = None;
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -114,7 +116,7 @@ mod tests {
 
         // Create and execute action
         let mut action = AddShapeAction::new(layer_id, shape, object);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify shape and object were added
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -129,7 +131,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify shape and object were removed
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -157,7 +159,7 @@ mod tests {
         assert_eq!(action.description(), "Add shape");
 
         // Execute
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
             assert_eq!(layer.shapes.len(), 1);
@@ -180,8 +182,8 @@ mod tests {
 
         // Execute twice - shapes are stored in HashMap (keyed by ID, so same shape overwrites)
         // while shape_instances are stored in Vec (so duplicates accumulate)
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
             // Shapes use HashMap keyed by shape.id, so same shape overwrites = 1
@@ -209,8 +211,8 @@ mod tests {
         let mut action1 = AddShapeAction::new(layer_id, shape1, object1);
         let mut action2 = AddShapeAction::new(layer_id, shape2, object2);
 
-        action1.execute(&mut document);
+        action1.execute(&mut document).unwrap();
-        action2.execute(&mut document);
+        action2.execute(&mut document).unwrap();
 
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
             // Two different shapes = 2 entries in HashMap

lightningbeam-ui/lightningbeam-core/src/actions/move_clip_instances.rs

@@ -26,7 +26,7 @@ impl MoveClipInstancesAction {
 }
 
 impl Action for MoveClipInstancesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         // Expand moves to include grouped instances
         let mut expanded_moves = self.layer_moves.clone();
         let mut already_processed = std::collections::HashSet::new();
@@ -71,16 +71,68 @@ impl Action for MoveClipInstancesAction {
             }
         }
 
-        // Store expanded moves for rollback
+        // Auto-adjust moves to avoid overlaps
-        self.layer_moves = expanded_moves.clone();
+        let mut adjusted_moves: HashMap<Uuid, Vec<(Uuid, f64, f64)>> = HashMap::new();
 
-        // Apply all moves (including expanded)
         for (layer_id, moves) in &expanded_moves {
-            let layer = match document.get_layer_mut(layer_id) {
+            let layer = document.get_layer(layer_id)
-                Some(l) => l,
+                .ok_or_else(|| format!("Layer {} not found", layer_id))?;
-                None => continue,
+
+            // Vector layers don't need adjustment
+            if matches!(layer, AnyLayer::Vector(_)) {
+                adjusted_moves.insert(*layer_id, moves.clone());
+                continue;
+            }
+
+            let mut adjusted_layer_moves = Vec::new();
+
+            for (instance_id, old_start, new_start) in moves {
+                // Get the instance to calculate its duration
+                let clip_instances = match layer {
+                    AnyLayer::Audio(al) => &al.clip_instances,
+                    AnyLayer::Video(vl) => &vl.clip_instances,
+                    AnyLayer::Vector(vl) => &vl.clip_instances,
                 };
 
+                let instance = clip_instances.iter()
+                    .find(|ci| &ci.id == instance_id)
+                    .ok_or_else(|| format!("Instance {} not found", instance_id))?;
+
+                let clip_duration = document.get_clip_duration(&instance.clip_id)
+                    .ok_or_else(|| format!("Clip {} not found", instance.clip_id))?;
+
+                let trim_start = instance.trim_start;
+                let trim_end = instance.trim_end.unwrap_or(clip_duration);
+                let effective_duration = trim_end - trim_start;
+
+                // Find nearest valid position, excluding this instance from overlap checks
+                let adjusted_start = document.find_nearest_valid_position(
+                    layer_id,
+                    *new_start,
+                    effective_duration,
+                    Some(instance_id),
+                );
+
+                if let Some(valid_start) = adjusted_start {
+                    adjusted_layer_moves.push((*instance_id, *old_start, valid_start));
+                } else {
+                    return Err(format!(
+                        "Cannot move clip: no valid position found on layer"
+                    ));
+                }
+            }
+
+            adjusted_moves.insert(*layer_id, adjusted_layer_moves);
+        }
+
+        // Store adjusted moves for rollback
+        self.layer_moves = adjusted_moves.clone();
+
+        // Apply all adjusted moves
+        for (layer_id, moves) in &adjusted_moves {
+            let layer = document.get_layer_mut(layer_id)
+                .ok_or_else(|| format!("Layer {} not found", layer_id))?;
+
             // Get mutable reference to clip_instances for this layer type
             let clip_instances = match layer {
                 AnyLayer::Vector(vl) => &mut vl.clip_instances,
@@ -96,14 +148,14 @@ impl Action for MoveClipInstancesAction {
                 }
             }
         }
+
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         for (layer_id, moves) in &self.layer_moves {
-            let layer = match document.get_layer_mut(layer_id) {
+            let layer = document.get_layer_mut(layer_id)
-                Some(l) => l,
+                .ok_or_else(|| format!("Layer {} not found", layer_id))?;
-                None => continue,
-            };
 
             // Get mutable reference to clip_instances for this layer type
             let clip_instances = match layer {
@@ -120,6 +172,8 @@ impl Action for MoveClipInstancesAction {
                 }
             }
         }
+
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -296,7 +350,7 @@ mod tests {
         let mut action = MoveClipInstancesAction::new(layer_moves);
 
         // Execute
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify position changed
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -309,7 +363,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify position restored
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {

lightningbeam-ui/lightningbeam-core/src/actions/move_objects.rs

@@ -34,10 +34,10 @@ impl MoveShapeInstancesAction {
 }
 
 impl Action for MoveShapeInstancesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         let layer = match document.get_layer_mut(&self.layer_id) {
             Some(l) => l,
-            None => return,
+            None => return Ok(()),
         };
 
         if let AnyLayer::Vector(vector_layer) = layer {
@@ -48,12 +48,13 @@ impl Action for MoveShapeInstancesAction {
                 });
             }
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         let layer = match document.get_layer_mut(&self.layer_id) {
             Some(l) => l,
-            None => return,
+            None => return Ok(()),
         };
 
         if let AnyLayer::Vector(vector_layer) = layer {
@@ -64,6 +65,7 @@ impl Action for MoveShapeInstancesAction {
                 });
             }
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -109,7 +111,7 @@ mod tests {
         let mut action = MoveShapeInstancesAction::new(layer_id, positions);
 
         // Execute
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify position changed
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -119,7 +121,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify position restored
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {

lightningbeam-ui/lightningbeam-core/src/actions/paint_bucket.rs

@@ -68,7 +68,7 @@ impl PaintBucketAction {
 }
 
 impl Action for PaintBucketAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         println!("=== PaintBucketAction::execute ===");
 
         // Optimization: Check if we're clicking on an existing shape first
@@ -116,7 +116,7 @@ impl Action for PaintBucketAction {
                             println!("Updated shape fill color");
                         }
 
-                        return; // Done! No need to create a new shape
+                        return Ok(()); // Done! No need to create a new shape
                     }
                 }
             }
@@ -131,7 +131,7 @@ impl Action for PaintBucketAction {
 
         if all_curves.is_empty() {
             println!("No curves found, returning");
-            return;
+            return Ok(());
         }
 
         // Step 2: Build planar graph
@@ -176,14 +176,15 @@ impl Action for PaintBucketAction {
         }
 
         println!("=== Paint Bucket Complete: Face filled with curves ===");
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         // Remove the created shape and object if they exist
         if let (Some(shape_id), Some(object_id)) = (self.created_shape_id, self.created_shape_instance_id) {
             let layer = match document.get_layer_mut(&self.layer_id) {
                 Some(l) => l,
-                None => return,
+                None => return Ok(()),
             };
 
             if let AnyLayer::Vector(vector_layer) = layer {
@@ -194,6 +195,7 @@ impl Action for PaintBucketAction {
             self.created_shape_id = None;
             self.created_shape_instance_id = None;
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -331,7 +333,7 @@ mod tests {
             GapHandlingMode::BridgeSegment,
         );
 
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify a filled shape was created
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -343,7 +345,7 @@ mod tests {
         }
 
         // Test rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
             // Should only have original shape

lightningbeam-ui/lightningbeam-core/src/actions/set_document_properties.rs

@@ -88,7 +88,7 @@ impl SetDocumentPropertiesAction {
 }
 
 impl Action for SetDocumentPropertiesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         // Store old value if not already stored
         if self.old_value.is_none() {
             self.old_value = Some(self.get_current_value(document));
@@ -97,12 +97,14 @@ impl Action for SetDocumentPropertiesAction {
         // Apply new value
         let new_value = self.property.value();
         self.apply_value(document, new_value);
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         if let Some(old_value) = self.old_value {
             self.apply_value(document, old_value);
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -126,10 +128,10 @@ mod tests {
         document.width = 1920.0;
 
         let mut action = SetDocumentPropertiesAction::set_width(1280.0);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
         assert_eq!(document.width, 1280.0);
 
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
         assert_eq!(document.width, 1920.0);
     }
 
@@ -139,10 +141,10 @@ mod tests {
         document.height = 1080.0;
 
         let mut action = SetDocumentPropertiesAction::set_height(720.0);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
         assert_eq!(document.height, 720.0);
 
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
         assert_eq!(document.height, 1080.0);
     }
 
@@ -152,10 +154,10 @@ mod tests {
         document.duration = 10.0;
 
         let mut action = SetDocumentPropertiesAction::set_duration(30.0);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
         assert_eq!(document.duration, 30.0);
 
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
         assert_eq!(document.duration, 10.0);
     }
 
@@ -165,10 +167,10 @@ mod tests {
         document.framerate = 30.0;
 
         let mut action = SetDocumentPropertiesAction::set_framerate(60.0);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
         assert_eq!(document.framerate, 60.0);
 
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
         assert_eq!(document.framerate, 30.0);
     }
 

lightningbeam-ui/lightningbeam-core/src/actions/set_instance_properties.rs

@@ -109,7 +109,7 @@ impl SetInstancePropertiesAction {
 }
 
 impl Action for SetInstancePropertiesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         let new_value = self.property.value();
         let layer_id = self.layer_id;
 
@@ -140,14 +140,16 @@ impl Action for SetInstancePropertiesAction {
         for (instance_id, _) in &self.instance_changes {
             self.apply_to_instance(document, instance_id, new_value);
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         for (instance_id, old_value) in &self.instance_changes {
             if let Some(value) = old_value {
                 self.apply_to_instance(document, instance_id, *value);
             }
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -195,7 +197,7 @@ mod tests {
             instance_id,
             InstancePropertyChange::X(50.0),
         );
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify position changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -205,7 +207,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -233,7 +235,7 @@ mod tests {
             instance_id,
             InstancePropertyChange::Rotation(45.0),
         );
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify rotation changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -242,7 +244,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -270,7 +272,7 @@ mod tests {
             instance_id,
             InstancePropertyChange::Opacity(0.5),
         );
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify opacity changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -279,7 +281,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -314,7 +316,7 @@ mod tests {
             vec![instance1_id, instance2_id],
             InstancePropertyChange::ScaleX(2.0),
         );
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify both changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -323,7 +325,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify both restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {

lightningbeam-ui/lightningbeam-core/src/actions/set_layer_properties.rs

@@ -74,7 +74,7 @@ impl SetLayerPropertiesAction {
 }
 
 impl Action for SetLayerPropertiesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         for (i, &layer_id) in self.layer_ids.iter().enumerate() {
             // Find the layer in the document
             if let Some(layer) = document.root_mut().get_child_mut(&layer_id) {
@@ -101,9 +101,10 @@ impl Action for SetLayerPropertiesAction {
                 }
             }
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         for (i, &layer_id) in self.layer_ids.iter().enumerate() {
             // Find the layer in the document
             if let Some(layer) = document.root_mut().get_child_mut(&layer_id) {
@@ -120,6 +121,7 @@ impl Action for SetLayerPropertiesAction {
                 }
             }
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -157,14 +159,14 @@ mod tests {
 
         // Create and execute action
         let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Volume(0.5));
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify volume changed
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.volume(), 0.5);
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify volume restored
         let layer_ref = document.root.get_child(&layer_id).unwrap();
@@ -183,13 +185,13 @@ mod tests {
 
         // Mute
         let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Muted(true));
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.muted(), true);
 
         // Unmute via rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.muted(), false);
@@ -208,14 +210,14 @@ mod tests {
             vec![id1, id2],
             LayerProperty::Soloed(true),
         );
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify both soloed
         assert_eq!(document.root.get_child(&id1).unwrap().soloed(), true);
         assert_eq!(document.root.get_child(&id2).unwrap().soloed(), true);
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify both unsoloed
         assert_eq!(document.root.get_child(&id1).unwrap().soloed(), false);
@@ -234,13 +236,13 @@ mod tests {
 
         // Lock
         let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Locked(true));
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.locked(), true);
 
         // Unlock via rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.locked(), false);
@@ -258,13 +260,13 @@ mod tests {
 
         // Set opacity to 0.5
         let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Opacity(0.5));
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.opacity(), 0.5);
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.opacity(), 1.0);
@@ -282,13 +284,13 @@ mod tests {
 
         // Hide
         let mut action = SetLayerPropertiesAction::new(layer_id, LayerProperty::Visible(false));
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.visible(), false);
 
         // Show via rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         let layer_ref = document.root.get_child(&layer_id).unwrap();
         assert_eq!(layer_ref.visible(), true);
@@ -307,14 +309,14 @@ mod tests {
             vec![id1, id2],
             LayerProperty::Locked(true),
         );
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify both locked
         assert_eq!(document.root.get_child(&id1).unwrap().locked(), true);
         assert_eq!(document.root.get_child(&id2).unwrap().locked(), true);
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify both unlocked
         assert_eq!(document.root.get_child(&id1).unwrap().locked(), false);
@@ -334,14 +336,14 @@ mod tests {
             vec![id1, id2],
             LayerProperty::Opacity(0.25),
         );
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify both have reduced opacity
         assert_eq!(document.root.get_child(&id1).unwrap().opacity(), 0.25);
         assert_eq!(document.root.get_child(&id2).unwrap().opacity(), 0.25);
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify both restored to 1.0
         assert_eq!(document.root.get_child(&id1).unwrap().opacity(), 1.0);
@@ -386,7 +388,7 @@ mod tests {
         let mut action = SetLayerPropertiesAction::new(fake_id, LayerProperty::Locked(true));
 
         // Should not panic
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
     }
 }

lightningbeam-ui/lightningbeam-core/src/actions/set_shape_properties.rs

@@ -60,7 +60,7 @@ impl SetShapePropertiesAction {
 }
 
 impl Action for SetShapePropertiesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         if let Some(layer) = document.get_layer_mut(&self.layer_id) {
             if let AnyLayer::Vector(vector_layer) = layer {
                 if let Some(shape) = vector_layer.shapes.get_mut(&self.shape_id) {
@@ -107,9 +107,10 @@ impl Action for SetShapePropertiesAction {
                 }
             }
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         if let Some(old_value) = &self.old_value {
             if let Some(layer) = document.get_layer_mut(&self.layer_id) {
                 if let AnyLayer::Vector(vector_layer) = layer {
@@ -131,6 +132,7 @@ impl Action for SetShapePropertiesAction {
                 }
             }
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -187,7 +189,7 @@ mod tests {
         // Create and execute action
         let new_color = Some(ShapeColor::rgb(0, 255, 0));
         let mut action = SetShapePropertiesAction::set_fill_color(layer_id, shape_id, new_color);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify color changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -196,7 +198,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -224,7 +226,7 @@ mod tests {
 
         // Create and execute action
         let mut action = SetShapePropertiesAction::set_stroke_width(layer_id, shape_id, 5.0);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify width changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -233,7 +235,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {

lightningbeam-ui/lightningbeam-core/src/actions/transform_clip_instances.rs

@@ -29,10 +29,10 @@ impl TransformClipInstancesAction {
 }
 
 impl Action for TransformClipInstancesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         let layer = match document.get_layer_mut(&self.layer_id) {
             Some(l) => l,
-            None => return,
+            None => return Ok(()),
         };
 
         // Get mutable reference to clip_instances for this layer type
@@ -48,12 +48,13 @@ impl Action for TransformClipInstancesAction {
                 clip_instance.transform = new.clone();
             }
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         let layer = match document.get_layer_mut(&self.layer_id) {
             Some(l) => l,
-            None => return,
+            None => return Ok(()),
         };
 
         // Get mutable reference to clip_instances for this layer type
@@ -69,6 +70,7 @@ impl Action for TransformClipInstancesAction {
                 clip_instance.transform = old.clone();
             }
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -108,7 +110,7 @@ mod tests {
         let mut action = TransformClipInstancesAction::new(layer_id, transforms);
 
         // Execute action
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify transform changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -120,7 +122,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify transform restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -153,7 +155,7 @@ mod tests {
         transforms.insert(instance_id, (old_transform, new_transform));
 
         let mut action = TransformClipInstancesAction::new(layer_id, transforms);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify
         if let Some(AnyLayer::Audio(al)) = document.get_layer_mut(&layer_id) {
@@ -194,7 +196,7 @@ mod tests {
         transforms.insert(instance_id, (old_transform, new_transform));
 
         let mut action = TransformClipInstancesAction::new(layer_id, transforms);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify rotation and scale
         if let Some(AnyLayer::Video(vl)) = document.get_layer_mut(&layer_id) {
@@ -240,7 +242,7 @@ mod tests {
         );
 
         let mut action = TransformClipInstancesAction::new(layer_id, transforms);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify both transformed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -256,7 +258,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify both restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -287,8 +289,8 @@ mod tests {
         let mut action = TransformClipInstancesAction::new(fake_layer_id, transforms);
 
         // Should not panic, just return early
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
     }
 
     #[test]

lightningbeam-ui/lightningbeam-core/src/actions/transform_objects.rs

@@ -30,7 +30,7 @@ impl TransformShapeInstancesAction {
 }
 
 impl Action for TransformShapeInstancesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         if let Some(layer) = document.get_layer_mut(&self.layer_id) {
             if let AnyLayer::Vector(vector_layer) = layer {
                 for (shape_instance_id, (_old, new)) in &self.shape_instance_transforms {
@@ -40,9 +40,10 @@ impl Action for TransformShapeInstancesAction {
                 }
             }
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         if let Some(layer) = document.get_layer_mut(&self.layer_id) {
             if let AnyLayer::Vector(vector_layer) = layer {
                 for (shape_instance_id, (old, _new)) in &self.shape_instance_transforms {
@@ -52,6 +53,7 @@ impl Action for TransformShapeInstancesAction {
                 }
             }
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -89,7 +91,7 @@ mod tests {
         let mut action = TransformShapeInstancesAction::new(layer_id, transforms);
 
         // Execute
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify transform changed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -101,7 +103,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -140,7 +142,7 @@ mod tests {
         transforms.insert(instance_id, (old_transform, new_transform));
 
         let mut action = TransformShapeInstancesAction::new(layer_id, transforms);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -187,7 +189,7 @@ mod tests {
         );
 
         let mut action = TransformShapeInstancesAction::new(layer_id, transforms);
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify both transformed
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -203,7 +205,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify both restored
         if let Some(AnyLayer::Vector(vl)) = document.get_layer_mut(&layer_id) {
@@ -234,8 +236,8 @@ mod tests {
         let mut action = TransformShapeInstancesAction::new(fake_layer_id, transforms);
 
         // Should not panic
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
     }
 
     #[test]
@@ -254,8 +256,8 @@ mod tests {
         let mut action = TransformShapeInstancesAction::new(layer_id, transforms);
 
         // Should not panic - just silently skip nonexistent instance
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
     }
 
     #[test]
@@ -276,8 +278,8 @@ mod tests {
         let mut action = TransformShapeInstancesAction::new(layer_id, transforms);
 
         // Should not panic - action only operates on vector layers
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
     }
 
     #[test]

lightningbeam-ui/lightningbeam-core/src/actions/trim_clip_instances.rs

@@ -62,7 +62,7 @@ impl TrimClipInstancesAction {
 }
 
 impl Action for TrimClipInstancesAction {
-    fn execute(&mut self, document: &mut Document) {
+    fn execute(&mut self, document: &mut Document) -> Result<(), String> {
         // Expand trims to include grouped instances
         let mut expanded_trims = self.layer_trims.clone();
         let mut already_processed = std::collections::HashSet::new();
@@ -155,11 +155,103 @@ impl Action for TrimClipInstancesAction {
             }
         }
 
-        // Store expanded trims for rollback
+        // Auto-clamp trims to avoid overlaps when extending clips
-        self.layer_trims = expanded_trims.clone();
+        let mut clamped_trims: HashMap<Uuid, Vec<(Uuid, TrimType, TrimData, TrimData)>> = HashMap::new();
 
-        // Apply all trims (including expanded)
         for (layer_id, trims) in &expanded_trims {
+            let layer = document.get_layer(layer_id)
+                .ok_or_else(|| format!("Layer {} not found", layer_id))?;
+
+            // Only validate for audio/video layers
+            let should_validate = matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_));
+
+            let mut clamped_layer_trims = Vec::new();
+
+            for (instance_id, trim_type, old, new) in trims {
+                let clip_instances = match layer {
+                    AnyLayer::Audio(al) => &al.clip_instances,
+                    AnyLayer::Video(vl) => &vl.clip_instances,
+                    AnyLayer::Vector(vl) => &vl.clip_instances,
+                };
+
+                let instance = clip_instances.iter()
+                    .find(|ci| &ci.id == instance_id)
+                    .ok_or_else(|| format!("Instance {} not found", instance_id))?;
+
+                let clip_duration = document.get_clip_duration(&instance.clip_id)
+                    .ok_or_else(|| format!("Clip {} not found", instance.clip_id))?;
+
+                let mut clamped_new = new.clone();
+
+                match trim_type {
+                    TrimType::TrimLeft => {
+                        if let (Some(old_trim), Some(new_trim), Some(old_timeline), Some(new_timeline)) =
+                            (old.trim_value, new.trim_value, old.timeline_start, new.timeline_start)
+                        {
+                            // If extending to the left (new_trim < old_trim)
+                            if should_validate && new_trim < old_trim {
+                                // Find the maximum we can extend left
+                                let max_extend = document.find_max_trim_extend_left(
+                                    layer_id,
+                                    instance_id,
+                                    instance.timeline_start,
+                                );
+
+                                // Calculate how much we want to extend
+                                let desired_extend = old_trim - new_trim;
+
+                                // Clamp to max allowed
+                                let actual_extend = desired_extend.min(max_extend);
+                                let clamped_trim_start = old_trim - actual_extend;
+                                let clamped_timeline_start = old_timeline - actual_extend;
+
+                                clamped_new = TrimData::left(clamped_trim_start, clamped_timeline_start);
+                            }
+                        }
+                    }
+                    TrimType::TrimRight => {
+                        let old_trim_end = old.trim_value.unwrap_or(clip_duration);
+                        let new_trim_end = new.trim_value.unwrap_or(clip_duration);
+
+                        // If extending to the right (new_trim_end > old_trim_end)
+                        if should_validate && new_trim_end > old_trim_end {
+                            // Calculate current effective duration
+                            let current_effective_duration = old_trim_end - instance.trim_start;
+
+                            // Find the maximum we can extend right
+                            let max_extend = document.find_max_trim_extend_right(
+                                layer_id,
+                                instance_id,
+                                instance.timeline_start,
+                                current_effective_duration,
+                            );
+
+                            // Calculate how much we want to extend
+                            let desired_extend = new_trim_end - old_trim_end;
+
+                            // Clamp to max allowed
+                            let actual_extend = desired_extend.min(max_extend);
+                            let clamped_trim_end = old_trim_end + actual_extend;
+
+                            // Don't exceed clip duration
+                            let final_trim_end = clamped_trim_end.min(clip_duration);
+
+                            clamped_new = TrimData::right(Some(final_trim_end));
+                        }
+                    }
+                }
+
+                clamped_layer_trims.push((*instance_id, *trim_type, old.clone(), clamped_new));
+            }
+
+            clamped_trims.insert(*layer_id, clamped_layer_trims);
+        }
+
+        // Store clamped trims for rollback
+        self.layer_trims = clamped_trims.clone();
+
+        // Apply all clamped trims
+        for (layer_id, trims) in &clamped_trims {
             let layer = match document.get_layer_mut(layer_id) {
                 Some(l) => l,
                 None => continue,
@@ -192,9 +284,10 @@ impl Action for TrimClipInstancesAction {
                 }
             }
         }
+        Ok(())
     }
 
-    fn rollback(&mut self, document: &mut Document) {
+    fn rollback(&mut self, document: &mut Document) -> Result<(), String> {
         for (layer_id, trims) in &self.layer_trims {
             let layer = match document.get_layer_mut(layer_id) {
                 Some(l) => l,
@@ -228,6 +321,7 @@ impl Action for TrimClipInstancesAction {
                 }
             }
         }
+        Ok(())
     }
 
     fn description(&self) -> String {
@@ -427,7 +521,7 @@ mod tests {
         let mut action = TrimClipInstancesAction::new(layer_trims);
 
         // Execute
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify trim applied
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -441,7 +535,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -486,7 +580,7 @@ mod tests {
         let mut action = TrimClipInstancesAction::new(layer_trims);
 
         // Execute
-        action.execute(&mut document);
+        action.execute(&mut document).unwrap();
 
         // Verify trim applied
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {
@@ -499,7 +593,7 @@ mod tests {
         }
 
         // Rollback
-        action.rollback(&mut document);
+        action.rollback(&mut document).unwrap();
 
         // Verify restored
         if let Some(AnyLayer::Vector(layer)) = document.get_layer(&layer_id) {

lightningbeam-ui/lightningbeam-core/src/document.rs

@@ -333,6 +333,290 @@ impl Document {
     pub fn remove_image_asset(&mut self, id: &Uuid) -> Option<ImageAsset> {
         self.image_assets.remove(id)
     }
+
+    // === CLIP OVERLAP DETECTION METHODS ===
+
+    /// Get the duration of any clip type by ID
+    ///
+    /// Searches through all clip libraries to find the clip and return its duration
+    pub fn get_clip_duration(&self, clip_id: &Uuid) -> Option<f64> {
+        if let Some(clip) = self.vector_clips.get(clip_id) {
+            Some(clip.duration)
+        } else if let Some(clip) = self.video_clips.get(clip_id) {
+            Some(clip.duration)
+        } else if let Some(clip) = self.audio_clips.get(clip_id) {
+            Some(clip.duration)
+        } else {
+            None
+        }
+    }
+
+    /// Calculate the end time of a clip instance on the timeline
+    pub fn get_clip_instance_end_time(&self, layer_id: &Uuid, instance_id: &Uuid) -> Option<f64> {
+        let layer = self.get_layer(layer_id)?;
+
+        // Find the clip instance
+        let instances = match layer {
+            AnyLayer::Audio(audio) => &audio.clip_instances,
+            AnyLayer::Video(video) => &video.clip_instances,
+            AnyLayer::Vector(vector) => &vector.clip_instances,
+        };
+
+        let instance = instances.iter().find(|inst| &inst.id == instance_id)?;
+        let clip_duration = self.get_clip_duration(&instance.clip_id)?;
+
+        let trim_start = instance.trim_start;
+        let trim_end = instance.trim_end.unwrap_or(clip_duration);
+        let effective_duration = trim_end - trim_start;
+
+        Some(instance.timeline_start + effective_duration)
+    }
+
+    /// Check if a time range overlaps with any existing clip on the layer
+    ///
+    /// Returns (overlaps, conflicting_instance_id)
+    ///
+    /// Only checks audio and video layers - vector/MIDI layers return false
+    pub fn check_overlap_on_layer(
+        &self,
+        layer_id: &Uuid,
+        start_time: f64,
+        end_time: f64,
+        exclude_instance_id: Option<&Uuid>,
+    ) -> (bool, Option<Uuid>) {
+        let Some(layer) = self.get_layer(layer_id) else {
+            return (false, None);
+        };
+
+        // Only check audio and video layers
+        if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) {
+            return (false, None);
+        }
+
+        let instances = match layer {
+            AnyLayer::Audio(audio) => &audio.clip_instances,
+            AnyLayer::Video(video) => &video.clip_instances,
+            AnyLayer::Vector(vector) => &vector.clip_instances,
+        };
+
+        for instance in instances {
+            // Skip the instance we're checking against itself
+            if let Some(exclude_id) = exclude_instance_id {
+                if &instance.id == exclude_id {
+                    continue;
+                }
+            }
+
+            // Calculate instance end time
+            let Some(clip_duration) = self.get_clip_duration(&instance.clip_id) else {
+                continue;
+            };
+
+            let instance_start = instance.timeline_start;
+            let trim_start = instance.trim_start;
+            let trim_end = instance.trim_end.unwrap_or(clip_duration);
+            let instance_end = instance_start + (trim_end - trim_start);
+
+            // Check overlap: start_a < end_b AND start_b < end_a
+            if start_time < instance_end && instance_start < end_time {
+                return (true, Some(instance.id));
+            }
+        }
+
+        (false, None)
+    }
+
+    /// Find the nearest valid position for a clip on a layer to avoid overlaps
+    ///
+    /// Returns adjusted timeline_start, or None if no valid position exists
+    ///
+    /// Strategy: Prefers snapping to the right (later in timeline) over left
+    pub fn find_nearest_valid_position(
+        &self,
+        layer_id: &Uuid,
+        desired_start: f64,
+        clip_duration: f64,
+        exclude_instance_id: Option<&Uuid>,
+    ) -> Option<f64> {
+        let layer = self.get_layer(layer_id)?;
+
+        // Vector/MIDI layers don't need adjustment
+        if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) {
+            return Some(desired_start);
+        }
+
+        // Check if desired position is already valid
+        let desired_end = desired_start + clip_duration;
+        let (overlaps, _) = self.check_overlap_on_layer(layer_id, desired_start, desired_end, exclude_instance_id);
+        if !overlaps {
+            return Some(desired_start);
+        }
+
+        // Collect all existing clip time ranges on this layer
+        let instances = match layer {
+            AnyLayer::Audio(audio) => &audio.clip_instances,
+            AnyLayer::Video(video) => &video.clip_instances,
+            _ => return Some(desired_start), // Shouldn't reach here
+        };
+
+        let mut occupied_ranges: Vec<(f64, f64, Uuid)> = Vec::new();
+        for instance in instances {
+            if let Some(exclude_id) = exclude_instance_id {
+                if &instance.id == exclude_id {
+                    continue;
+                }
+            }
+
+            if let Some(clip_dur) = self.get_clip_duration(&instance.clip_id) {
+                let inst_start = instance.timeline_start;
+                let trim_start = instance.trim_start;
+                let trim_end = instance.trim_end.unwrap_or(clip_dur);
+                let inst_end = inst_start + (trim_end - trim_start);
+                occupied_ranges.push((inst_start, inst_end, instance.id));
+            }
+        }
+
+        // Sort by start time
+        occupied_ranges.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap_or(std::cmp::Ordering::Equal));
+
+        // Find the clip we're overlapping with
+        for (occupied_start, occupied_end, _) in &occupied_ranges {
+            if desired_start < *occupied_end && *occupied_start < desired_end {
+                // Try snapping to the right (after this clip)
+                let snap_right = *occupied_end;
+                let snap_right_end = snap_right + clip_duration;
+
+                let (overlaps_right, _) = self.check_overlap_on_layer(
+                    layer_id,
+                    snap_right,
+                    snap_right_end,
+                    exclude_instance_id,
+                );
+
+                if !overlaps_right {
+                    return Some(snap_right);
+                }
+
+                // Try snapping to the left (before this clip)
+                let snap_left = occupied_start - clip_duration;
+                if snap_left >= 0.0 {
+                    let (overlaps_left, _) = self.check_overlap_on_layer(
+                        layer_id,
+                        snap_left,
+                        *occupied_start,
+                        exclude_instance_id,
+                    );
+
+                    if !overlaps_left {
+                        return Some(snap_left);
+                    }
+                }
+            }
+        }
+
+        // If no gap found, try placing at timeline start
+        if occupied_ranges.is_empty() || occupied_ranges[0].0 >= clip_duration {
+            return Some(0.0);
+        }
+
+        // No valid position found
+        None
+    }
+
+    /// Find the maximum amount we can extend a clip to the left without overlapping
+    ///
+    /// Returns the distance to the nearest clip to the left, or the distance to
+    /// timeline start (0.0) if no clips exist to the left.
+    pub fn find_max_trim_extend_left(
+        &self,
+        layer_id: &Uuid,
+        instance_id: &Uuid,
+        current_timeline_start: f64,
+    ) -> f64 {
+        let Some(layer) = self.get_layer(layer_id) else {
+            return current_timeline_start; // No limit if layer not found
+        };
+
+        // Only check audio and video layers
+        if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) {
+            return current_timeline_start; // No limit for vector layers
+        };
+
+        // Find the nearest clip to the left
+        let mut nearest_end = 0.0; // Can extend to timeline start by default
+
+        let instances = match layer {
+            AnyLayer::Audio(audio) => &audio.clip_instances,
+            AnyLayer::Video(video) => &video.clip_instances,
+            AnyLayer::Vector(vector) => &vector.clip_instances,
+        };
+
+        for other in instances {
+            if &other.id == instance_id {
+                continue;
+            }
+
+            // Calculate other clip's end time
+            if let Some(clip_duration) = self.get_clip_duration(&other.clip_id) {
+                let trim_end = other.trim_end.unwrap_or(clip_duration);
+                let other_end = other.timeline_start + (trim_end - other.trim_start);
+
+                // If this clip is to the left and closer than current nearest
+                if other_end <= current_timeline_start && other_end > nearest_end {
+                    nearest_end = other_end;
+                }
+            }
+        }
+
+        current_timeline_start - nearest_end
+    }
+
+    /// Find the maximum amount we can extend a clip to the right without overlapping
+    ///
+    /// Returns the distance to the nearest clip to the right, or f64::MAX if no
+    /// clips exist to the right.
+    pub fn find_max_trim_extend_right(
+        &self,
+        layer_id: &Uuid,
+        instance_id: &Uuid,
+        current_timeline_start: f64,
+        current_effective_duration: f64,
+    ) -> f64 {
+        let Some(layer) = self.get_layer(layer_id) else {
+            return f64::MAX; // No limit if layer not found
+        };
+
+        // Only check audio and video layers
+        if !matches!(layer, AnyLayer::Audio(_) | AnyLayer::Video(_)) {
+            return f64::MAX; // No limit for vector layers
+        }
+
+        let instances = match layer {
+            AnyLayer::Audio(audio) => &audio.clip_instances,
+            AnyLayer::Video(video) => &video.clip_instances,
+            AnyLayer::Vector(vector) => &vector.clip_instances,
+        };
+
+        let mut nearest_start = f64::MAX;
+        let current_end = current_timeline_start + current_effective_duration;
+
+        for other in instances {
+            if &other.id == instance_id {
+                continue;
+            }
+
+            // If this clip is to the right and closer than current nearest
+            if other.timeline_start >= current_end && other.timeline_start < nearest_start {
+                nearest_start = other.timeline_start;
+            }
+        }
+
+        if nearest_start == f64::MAX {
+            f64::MAX // No clip to the right, can extend freely
+        } else {
+            nearest_start - current_timeline_start // Distance to next clip
+        }
+    }
 }
 
 #[cfg(test)]

lightningbeam-ui/lightningbeam-editor/src/main.rs

@@ -1024,10 +1024,10 @@ impl EditorApp {
                         Err(e) => eprintln!("Undo failed: {}", e),
                     }
                 } else {
-                    if self.action_executor.undo() {
+                    match self.action_executor.undo() {
-                        println!("Undid: {}", self.action_executor.redo_description().unwrap_or_default());
+                        Ok(true) => println!("Undid: {}", self.action_executor.redo_description().unwrap_or_default()),
-                    } else {
+                        Ok(false) => println!("Nothing to undo"),
-                        println!("Nothing to undo");
+                        Err(e) => eprintln!("Undo failed: {}", e),
                     }
                 }
             }
@@ -1046,10 +1046,10 @@ impl EditorApp {
                         Err(e) => eprintln!("Redo failed: {}", e),
                     }
                 } else {
-                    if self.action_executor.redo() {
+                    match self.action_executor.redo() {
-                        println!("Redid: {}", self.action_executor.undo_description().unwrap_or_default());
+                        Ok(true) => println!("Redid: {}", self.action_executor.undo_description().unwrap_or_default()),
-                    } else {
+                        Ok(false) => println!("Nothing to redo"),
-                        println!("Nothing to redo");
+                        Err(e) => eprintln!("Redo failed: {}", e),
                     }
                 }
             }

lightningbeam-ui/lightningbeam-editor/src/panes/stage.rs

@@ -1283,6 +1283,8 @@ pub struct StagePane {
     instance_id: u64,
     // Eyedropper state
     pending_eyedropper_sample: Option<(egui::Pos2, super::ColorMode)>,
+    // Last known viewport rect (for zoom-to-fit calculation)
+    last_viewport_rect: Option<egui::Rect>,
 }
 
 // Global counter for generating unique instance IDs
@@ -1301,6 +1303,7 @@ impl StagePane {
             last_pan_pos: None,
             instance_id,
             pending_eyedropper_sample: None,
+            last_viewport_rect: None,
         }
     }
 
@@ -1338,6 +1341,35 @@ impl StagePane {
         self.zoom = 1.0;
     }
 
+    /// Zoom to fit the canvas (document dimensions) in the available viewport
+    pub fn zoom_to_fit(&mut self, shared: &SharedPaneState) {
+        let document = shared.action_executor.document();
+
+        // Get document dimensions
+        let doc_width = document.width as f32;
+        let doc_height = document.height as f32;
+
+        // Get viewport size from last known rect
+        let viewport_size = if let Some(rect) = self.last_viewport_rect {
+            rect.size()
+        } else {
+            // Fallback if we don't have a rect yet
+            egui::vec2(800.0, 600.0)
+        };
+
+        // Calculate zoom to fit both width and height (no padding - use entire space)
+        let zoom_x = viewport_size.x / doc_width;
+        let zoom_y = viewport_size.y / doc_height;
+
+        // Use the smaller zoom to ensure both dimensions fit
+        self.zoom = zoom_x.min(zoom_y).clamp(0.1, 10.0);
+
+        // Center the document in the viewport
+        let canvas_center = egui::vec2(doc_width / 2.0, doc_height / 2.0) * self.zoom;
+        let viewport_center = viewport_size / 2.0;
+        self.pan_offset = viewport_center - canvas_center;
+    }
+
     /// Apply zoom while keeping the point under the mouse cursor stationary
     fn apply_zoom_at_point(&mut self, zoom_delta: f32, mouse_canvas_pos: egui::Vec2) {
         let old_zoom = self.zoom;
@@ -4124,6 +4156,23 @@ impl StagePane {
 }
 
 impl PaneRenderer for StagePane {
+    fn render_header(&mut self, ui: &mut egui::Ui, shared: &mut SharedPaneState) -> bool {
+        ui.horizontal(|ui| {
+            // Zoom to fit button
+            if ui.button("⊡ Fit").on_hover_text("Zoom to fit canvas in view").clicked() {
+                self.zoom_to_fit(shared);
+            }
+
+            ui.separator();
+
+            // Zoom level display
+            let text_style = shared.theme.style(".text-primary", ui.ctx());
+            let text_color = text_style.text_color.unwrap_or(egui::Color32::from_gray(200));
+            ui.colored_label(text_color, format!("Zoom: {:.0}%", self.zoom * 100.0));
+        });
+        true
+    }
+
     fn render_content(
         &mut self,
         ui: &mut egui::Ui,
@@ -4131,6 +4180,9 @@ impl PaneRenderer for StagePane {
         _path: &NodePath,
         shared: &mut SharedPaneState,
     ) {
+        // Store viewport rect for zoom-to-fit calculation
+        self.last_viewport_rect = Some(rect);
+
         // Check for completed eyedropper samples from GPU readback and apply them
         if let Ok(mut results) = EYEDROPPER_RESULTS
             .get_or_init(|| Arc::new(Mutex::new(std::collections::HashMap::new())))

lightningbeam-ui/lightningbeam-editor/src/panes/timeline.rs

@@ -746,7 +746,7 @@ impl TimelinePane {
                         lightningbeam_core::layer::AudioLayerType::Sampled => ("Audio", egui::Color32::from_rgb(100, 180, 255)), // Blue
                     }
                 }
-                lightningbeam_core::layer::AnyLayer::Video(_) => ("Video", egui::Color32::from_rgb(255, 150, 100)), // Orange/Red
+                lightningbeam_core::layer::AnyLayer::Video(_) => ("Video", egui::Color32::from_rgb(180, 100, 255)), // Purple
             };
 
             // Color indicator bar on the left edge
@@ -1061,21 +1061,51 @@ impl TimelinePane {
                     let layer_data = layer.layer();
                     let mut instance_start = clip_instance.timeline_start;
 
-                    // Apply drag offset preview for selected clips
+                    // Apply drag offset preview for selected clips with snapping
                     let is_selected = selection.contains_clip_instance(&clip_instance.id);
 
                     if let Some(drag_type) = self.clip_drag_state {
                         if is_selected {
                             match drag_type {
                                 ClipDragType::Move => {
-                                    // Move: shift the entire clip along the timeline
+                                    // Move: shift the entire clip along the timeline with auto-snap preview
-                                    instance_start += self.drag_offset;
+                                    let desired_start = clip_instance.timeline_start + self.drag_offset;
+                                    let current_duration = instance_duration;
+
+                                    // Find snapped position for preview
+                                    let snapped_start = document
+                                        .find_nearest_valid_position(
+                                            &layer.id(),
+                                            desired_start,
+                                            current_duration,
+                                            Some(&clip_instance.id),
+                                        )
+                                        .unwrap_or(desired_start);
+
+                                    instance_start = snapped_start;
                                 }
                                 ClipDragType::TrimLeft => {
-                                    // Trim left: calculate new trim_start and clamp to valid range
+                                    // Trim left: calculate new trim_start with snap to adjacent clips
-                                    let new_trim_start = (clip_instance.trim_start + self.drag_offset)
+                                    let desired_trim_start = (clip_instance.trim_start + self.drag_offset)
                                         .max(0.0)
                                         .min(clip_duration);
+
+                                    let new_trim_start = if desired_trim_start < clip_instance.trim_start {
+                                        // Extending left - check for adjacent clips
+                                        let max_extend = document.find_max_trim_extend_left(
+                                            &layer.id(),
+                                            &clip_instance.id,
+                                            clip_instance.timeline_start,
+                                        );
+
+                                        let desired_extend = clip_instance.trim_start - desired_trim_start;
+                                        let actual_extend = desired_extend.min(max_extend);
+                                        clip_instance.trim_start - actual_extend
+                                    } else {
+                                        // Shrinking - no snap needed
+                                        desired_trim_start
+                                    };
+
                                     let actual_offset = new_trim_start - clip_instance.trim_start;
 
                                     // Move start and reduce duration by actual clamped offset
@@ -1089,11 +1119,32 @@ impl TimelinePane {
                                     }
                                 }
                                 ClipDragType::TrimRight => {
-                                    // Trim right: extend or reduce duration, clamped to available content
+                                    // Trim right: extend or reduce duration with snap to adjacent clips
-                                    let max_duration = clip_duration - clip_instance.trim_start;
+                                    let old_trim_end = clip_instance.trim_end.unwrap_or(clip_duration);
-                                    instance_duration = (instance_duration + self.drag_offset)
+                                    let desired_change = self.drag_offset;
-                                        .max(0.0)
+                                    let desired_trim_end = (old_trim_end + desired_change)
-                                        .min(max_duration);
+                                        .max(clip_instance.trim_start)
+                                        .min(clip_duration);
+
+                                    let new_trim_end = if desired_trim_end > old_trim_end {
+                                        // Extending right - check for adjacent clips
+                                        let current_duration = old_trim_end - clip_instance.trim_start;
+                                        let max_extend = document.find_max_trim_extend_right(
+                                            &layer.id(),
+                                            &clip_instance.id,
+                                            clip_instance.timeline_start,
+                                            current_duration,
+                                        );
+
+                                        let desired_extend = desired_trim_end - old_trim_end;
+                                        let actual_extend = desired_extend.min(max_extend);
+                                        old_trim_end + actual_extend
+                                    } else {
+                                        // Shrinking - no snap needed
+                                        desired_trim_end
+                                    };
+
+                                    instance_duration = (new_trim_end - clip_instance.trim_start).max(0.0);
                                 }
                             }
                         }
@@ -1128,8 +1179,8 @@ impl TimelinePane {
                                 }
                             }
                             lightningbeam_core::layer::AnyLayer::Video(_) => (
-                                egui::Color32::from_rgb(255, 150, 100), // Orange/Red
+                                egui::Color32::from_rgb(150, 80, 220), // Purple
-                                egui::Color32::from_rgb(255, 200, 150), // Bright orange/red
+                                egui::Color32::from_rgb(200, 150, 255), // Bright purple
                             ),
                         };
 
@@ -2081,8 +2132,31 @@ impl PaneRenderer for TimelinePane {
 
                         ui.painter().rect_filled(highlight_rect, 0.0, highlight_color);
 
-                        // Show drop time indicator
+                        // Show drop time indicator with snap preview
-                        let drop_time = self.x_to_time(pointer_pos.x - content_rect.min.x);
+                        let raw_drop_time = self.x_to_time(pointer_pos.x - content_rect.min.x).max(0.0);
+
+                        // Calculate snapped drop time for preview
+                        let drop_time = if is_compatible {
+                            // Get clip duration to calculate snapped position
+                            let clip_duration = {
+                                let doc = shared.action_executor.document();
+                                doc.get_clip_duration(&dragging.clip_id).unwrap_or(1.0)
+                            };
+
+                            // Find nearest valid position (auto-snap for preview)
+                            let snapped = shared.action_executor.document()
+                                .find_nearest_valid_position(
+                                    &layer.id(),
+                                    raw_drop_time,
+                                    clip_duration,
+                                    None,
+                                );
+
+                            snapped.unwrap_or(raw_drop_time)
+                        } else {
+                            raw_drop_time
+                        };
+
                         let drop_x = self.time_to_x(drop_time);
                         if drop_x >= 0.0 && drop_x <= content_rect.width() {
                             ui.painter().line_segment(
@@ -2106,8 +2180,7 @@ impl PaneRenderer for TimelinePane {
                                 let center_y = doc.height / 2.0;
 
                                 let mut clip_instance = ClipInstance::new(dragging.clip_id)
-                                    .with_timeline_start(drop_time)
+                                    .with_timeline_start(drop_time);
-                                    .with_position(center_x, center_y);
 
                                 // For video clips, scale to fill document dimensions
                                 if dragging.clip_type == DragClipType::Video {
@@ -2118,7 +2191,20 @@ impl PaneRenderer for TimelinePane {
 
                                         clip_instance.transform.scale_x = scale_x;
                                         clip_instance.transform.scale_y = scale_y;
+
+                                        // Position at (0, 0) to center the scaled video
+                                        // (scaled dimensions = document dimensions, so top-left at origin centers it)
+                                        clip_instance.transform.x = 0.0;
+                                        clip_instance.transform.y = 0.0;
+                                    } else {
+                                        // No dimensions available, use document center
+                                        clip_instance.transform.x = center_x;
+                                        clip_instance.transform.y = center_y;
                                     }
+                                } else {
+                                    // Non-video clips: center at document center
+                                    clip_instance.transform.x = center_x;
+                                    clip_instance.transform.y = center_y;
                                 }
 
                                 (center_x, center_y, clip_instance)