fix: Various Delete Function Fixes (#801)

Author: liksunrice

crates/koharu-app/src/history.rs

@@ -155,6 +155,68 @@ impl History {
     }
 
     fn push_undo(&mut self, op: Op) {
+        // Try to merge if the incoming op is a pipeline render operation
+        if let Some(prev) = self.undo_stack.back_mut() {
+            let should_merge = if let Op::Batch { label, .. } = &op
+                && (label.contains("renderer: page ") || label.contains("koharu-renderer: page "))
+            {
+                match prev {
+                    Op::Batch {
+                        label: prev_label, ..
+                    } => !prev_label.starts_with("undo:") && !prev_label.contains(": page "),
+                    _ => true,
+                }
+            } else {
+                false
+            };
+
+            if should_merge
+                && let Op::Batch {
+                    label: incoming_label,
+                    ops: incoming_ops,
+                } = op
+            {
+                match prev {
+                    Op::Batch { ops: prev_ops, .. } => {
+                        prev_ops.extend(incoming_ops);
+                    }
+                    single_op => {
+                        let prev_op = std::mem::replace(
+                            single_op,
+                            Op::Batch {
+                                ops: vec![],
+                                label: String::new(),
+                            },
+                        );
+                        if let Op::Batch { ops, label } = single_op {
+                            *ops = vec![
+                                prev_op,
+                                Op::Batch {
+                                    ops: incoming_ops,
+                                    label: incoming_label,
+                                },
+                            ];
+                            *label = match &ops[0] {
+                                Op::RemoveNode { .. } => "Delete block".to_string(),
+                                Op::UpdateNode { .. } => "Update block".to_string(),
+                                Op::AddNode { .. } => "Add block".to_string(),
+                                Op::RemovePage { .. } => "Delete page".to_string(),
+                                Op::AddPage { .. } => "Add page".to_string(),
+                                Op::UpdatePage { .. } => "Update page".to_string(),
+                                Op::ReorderPages { .. } => "Reorder pages".to_string(),
+                                Op::ReorderNodes { .. } => "Reorder blocks".to_string(),
+                                Op::UpdateProjectMeta { .. } => {
+                                    "Update project metadata".to_string()
+                                }
+                                Op::Batch { label: l, .. } => l.clone(),
+                            };
+                        }
+                    }
+                }
+                return;
+            }
+        }
+
         self.undo_stack.push_back(op);
         while self.undo_stack.len() > self.limit {
             self.undo_stack.pop_front();
@@ -219,3 +281,139 @@ pub fn replay(log_path: &Path, start_epoch: u64, scene: &mut Scene) -> Result<u6
     let _ = reader.seek(SeekFrom::End(0));
     Ok(epoch)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::time::{SystemTime, UNIX_EPOCH};
+
+    fn make_temp_history() -> (History, std::path::PathBuf) {
+        let mut temp_path = std::env::temp_dir();
+        let unique_id = SystemTime::now()
+            .duration_since(UNIX_EPOCH)
+            .unwrap()
+            .as_nanos();
+        temp_path.push(format!("test_history_{}.log", unique_id));
+        let history = History::open(&temp_path, 0).unwrap();
+        (history, temp_path)
+    }
+
+    #[test]
+    fn test_push_undo_merge_user_edit() {
+        let (mut history, path) = make_temp_history();
+
+        // 1. User action: "Update block"
+        let user_op = Op::Batch {
+            ops: vec![],
+            label: "Update block".to_string(),
+        };
+        history.push_undo(user_op);
+        assert_eq!(history.undo_stack.len(), 1);
+
+        // 2. Renderer operation
+        let render_op = Op::Batch {
+            ops: vec![],
+            label: "koharu-renderer: page 1".to_string(),
+        };
+        history.push_undo(render_op);
+
+        // Should merge: undo stack size remains 1
+        assert_eq!(history.undo_stack.len(), 1);
+        if let Some(Op::Batch { label, .. }) = history.undo_stack.back() {
+            assert_eq!(label, "Update block");
+        } else {
+            panic!("Expected Batch");
+        }
+
+        drop(history);
+        let _ = std::fs::remove_file(path);
+    }
+
+    #[test]
+    fn test_push_undo_no_merge_pipeline_step() {
+        let (mut history, path) = make_temp_history();
+
+        // 1. Pipeline step: "translation-gpt-4o: page 1"
+        let pipeline_op = Op::Batch {
+            ops: vec![],
+            label: "translation-gpt-4o: page 1".to_string(),
+        };
+        history.push_undo(pipeline_op);
+        assert_eq!(history.undo_stack.len(), 1);
+
+        // 2. Renderer operation
+        let render_op = Op::Batch {
+            ops: vec![],
+            label: "koharu-renderer: page 1".to_string(),
+        };
+        history.push_undo(render_op);
+
+        // Should NOT merge: undo stack size becomes 2
+        assert_eq!(history.undo_stack.len(), 2);
+
+        let first = &history.undo_stack[0];
+        let second = &history.undo_stack[1];
+
+        if let Op::Batch { label, .. } = first {
+            assert_eq!(label, "translation-gpt-4o: page 1");
+        } else {
+            panic!("Expected Batch");
+        }
+
+        if let Op::Batch { label, .. } = second {
+            assert_eq!(label, "koharu-renderer: page 1");
+        } else {
+            panic!("Expected Batch");
+        }
+
+        drop(history);
+        let _ = std::fs::remove_file(path);
+    }
+
+    #[test]
+    fn test_push_undo_merge_only_immediate_preceding_user_edit() {
+        let (mut history, path) = make_temp_history();
+
+        // 1. Pipeline step (first in history)
+        let pipeline_op = Op::Batch {
+            ops: vec![],
+            label: "translation-gpt-4o: page 1".to_string(),
+        };
+        history.push_undo(pipeline_op);
+
+        // 2. User edit (second in history, immediately preceding)
+        let user_op = Op::Batch {
+            ops: vec![],
+            label: "Update block".to_string(),
+        };
+        history.push_undo(user_op);
+
+        // 3. Renderer operation
+        let render_op = Op::Batch {
+            ops: vec![],
+            label: "koharu-renderer: page 1".to_string(),
+        };
+        history.push_undo(render_op);
+
+        // Undo stack should have 2 entries (pipeline step + merged user edit & render)
+        assert_eq!(history.undo_stack.len(), 2);
+
+        let first = &history.undo_stack[0];
+        let second = &history.undo_stack[1];
+
+        if let Op::Batch { label, .. } = first {
+            assert_eq!(label, "translation-gpt-4o: page 1");
+        } else {
+            panic!("Expected Batch");
+        }
+
+        if let Op::Batch { label, .. } = second {
+            assert_eq!(label, "Update block");
+        } else {
+            panic!("Expected Batch");
+        }
+
+        drop(history);
+        let _ = std::fs::remove_file(path);
+    }
+}

crates/koharu-ml/tests/README.md

@@ -1,10 +1,10 @@
-# Koharu ML tests
-
-To run test locally if you have CUDA enabled GPU, use the following command:
-
-```bash
-# llm tests only
-bun run cargo test --package koharu-ml --test llm --features cuda
-# all
-bun run cargo test --package koharu-ml --features cuda
+# Koharu ML tests
+
+To run test locally if you have CUDA enabled GPU, use the following command:
+
+```bash
+# llm tests only
+bun run cargo test --package koharu-ml --test llm --features cuda
+# all
+bun run cargo test --package koharu-ml --features cuda
 ```

crates/koharu/tauri.linux.conf.json

@@ -1,5 +1,5 @@
-{
-  "$schema": "../node_modules/@tauri-apps/cli/config.schema.json",
+{
+  "$schema": "../node_modules/@tauri-apps/cli/config.schema.json",
   "identifier": "Koharu",
   "build": {
     "features": [

ui/app/globals.css

@@ -145,4 +145,3 @@
     @apply select-text;
   }
 }
-

ui/components/MenuBar.tsx

@@ -24,7 +24,13 @@ import { useScene } from '@/hooks/useScene'
 import { getConfig, startPipeline } from '@/lib/api/default/default'
 import { isTauri, openExternalUrl } from '@/lib/backend'
 import { exportCurrentProjectAs, importPages } from '@/lib/io/pagesIo'
-import { clearSelectionOnCurrentPage, closeProject, redoOp, selectAllTextNodesOnCurrentPage, undoOp } from '@/lib/io/scene'
+import {
+  clearSelectionOnCurrentPage,
+  closeProject,
+  redoOp,
+  selectAllTextNodesOnCurrentPage,
+  undoOp,
+} from '@/lib/io/scene'
 import { formatShortcutForDisplay, getPlatform } from '@/lib/shortcutUtils'
 import { useEditorUiStore } from '@/lib/stores/editorUiStore'
 import { usePreferencesStore } from '@/lib/stores/preferencesStore'

ui/components/Navigator.tsx

@@ -4,6 +4,7 @@ import { useVirtualizer } from '@tanstack/react-virtual'
 import { LayoutGridIcon, Trash2Icon } from 'lucide-react'
 import { memo, useCallback, useMemo, useRef, useState } from 'react'
 import type React from 'react'
+import { useHotkeys } from 'react-hotkeys-hook'
 import { useTranslation } from 'react-i18next'
 
 import { PageManagerDialog } from '@/components/PageManagerDialog'
@@ -14,7 +15,6 @@ import { getGetPageThumbnailUrl } from '@/lib/api/default/default'
 import { applyOp } from '@/lib/io/scene'
 import { ops } from '@/lib/ops'
 import { useSelectionStore } from '@/lib/stores/selectionStore'
-import { useHotkeys } from 'react-hotkeys-hook'
 
 const THUMBNAIL_DPR =
   typeof window !== 'undefined' ? Math.min(Math.ceil(window.devicePixelRatio || 1), 3) : 2
@@ -126,7 +126,7 @@ export function Navigator() {
     (id: string) => {
       void handleDeletePages(new Set([id]))
     },
-    [handleDeletePages,],
+    [handleDeletePages],
   )
 
   const handleBatchDelete = useCallback(() => {

ui/components/canvas/TextBlockLayer.tsx

@@ -1,8 +1,7 @@
 'use client'
 
 import { useDrag } from '@use-gesture/react'
-import { useMemo, useRef } from 'react'
-import { useHotkeys } from 'react-hotkeys-hook'
+import { useEffect, useRef } from 'react'
 
 import { useBlobImage } from '@/hooks/useBlobData'
 import { useCurrentPage, useTextNodes, type TextNodeEntry } from '@/hooks/useCurrentPage'
@@ -31,11 +30,6 @@ export function TextBlockLayer({ showSprites, scale, style }: TextBlockLayerProp
   const mode = useEditorUiStore((s) => s.mode)
   const interactive = mode === 'select' || mode === 'block'
 
-  const hasSelection = useMemo(() => {
-    for (const id of selectedIds) if (id) return true
-    return false
-  }, [selectedIds])
-
   const updateTransform = async (id: string, t: Transform) => {
     if (!page) return
     const data: NodeDataPatch = {
@@ -111,6 +105,12 @@ function TextBlockItem({
   const edgeRef = useRef<ResizeEdge | null>(null)
   const isResizeRef = useRef(false)
 
+  useEffect(() => {
+    if (selected && boxRef.current) {
+      boxRef.current.focus()
+    }
+  }, [selected])
+
   const setBox = (x: number, y: number, w: number, h: number) => {
     const el = boxRef.current
     if (!el) return
@@ -128,6 +128,7 @@ function TextBlockItem({
       const additive = isAdditiveEvent(event)
       if (tap) {
         onSelect(node.id, additive)
+        boxRef.current?.focus()
         return
       }
       if (first) {
@@ -140,6 +141,7 @@ function TextBlockItem({
         // Keep multi-selection intact when dragging a node that's already selected;
         // otherwise this click is a single-select (unless the modifier is held).
         if (additive || !selected) onSelect(node.id, additive)
+        boxRef.current?.focus()
       }
       const { x: sx, y: sy, w: sw, h: sh } = dragStart.current
       const edge = edgeRef.current
@@ -208,6 +210,7 @@ function TextBlockItem({
     <div
       ref={boxRef}
       {...bind()}
+      tabIndex={-1}
       style={{
         position: 'absolute',
         top: 0,
@@ -219,17 +222,20 @@ function TextBlockItem({
         zIndex: selected ? 20 : 10,
         touchAction: 'none',
         cursor: interactive ? 'move' : 'default',
+        outline: 'none',
       }}
     >
       <div
-        className={`absolute inset-0 rounded-md ${selected
-          ? 'border-[3px] border-primary bg-primary/15'
-          : 'border-2 border-rose-400/60 bg-rose-400/5'
-          }`}
+        className={`absolute inset-0 rounded-md ${
+          selected
+            ? 'border-[3px] border-primary bg-primary/15'
+            : 'border-2 border-rose-400/60 bg-rose-400/5'
+        }`}
       />
       <div
-        className={`pointer-events-none absolute -top-1.5 -left-1.5 flex h-4 w-4 items-center justify-center rounded-full text-[9px] font-semibold text-white shadow ${selected ? 'bg-primary' : 'bg-rose-400'
-          }`}
+        className={`pointer-events-none absolute -top-1.5 -left-1.5 flex h-4 w-4 items-center justify-center rounded-full text-[9px] font-semibold text-white shadow ${
+          selected ? 'bg-primary' : 'bg-rose-400'
+        }`}
       >
         {index + 1}
       </div>