chore: code-review fixes (menu churn, split-mark range, mutex recovery)

src-tauri/src/lib.rs

@@ -613,6 +613,17 @@ struct ChildRegistry(Mutex<HashMap<String, Arc<ChildHandle>>>);
 #[derive(Default)]
 struct PendingDeepLink(Mutex<Option<String>>);
 
+/// Lock a `Mutex` without panicking on poisoning. These mutexes guard plain data
+/// (a process handle, a registry map, a pending path) that stays valid even if a
+/// thread panicked while holding the lock, so recovering the inner guard is the
+/// right call — far better than propagating a panic out of a process-spawn or
+/// deep-link path.
+fn lock_recover<T>(mutex: &Mutex<T>) -> std::sync::MutexGuard<'_, T> {
+    mutex
+        .lock()
+        .unwrap_or_else(|poisoned| poisoned.into_inner())
+}
+
 fn claude_projects_dir() -> Result<PathBuf, String> {
     let home = dirs::home_dir().ok_or_else(|| "Could not resolve home directory".to_string())?;
     Ok(home.join(".claude").join("projects"))
@@ -1270,11 +1281,7 @@ fn spawn_claude_resume(
     });
     {
         let registry = app.state::<ChildRegistry>();
-        registry
-            .0
-            .lock()
-            .unwrap()
-            .insert(token.clone(), handle.clone());
+        lock_recover(&registry.0).insert(token.clone(), handle.clone());
     }
 
     let token_for_thread = token.clone();
@@ -1295,7 +1302,12 @@ fn spawn_claude_resume(
             }
             let parsed: serde_json::Value = match serde_json::from_str(&line) {
                 Ok(v) => v,
-                Err(_) => continue,
+                Err(e) => {
+                    // A non-JSON line in the stream-json output is unexpected;
+                    // skip it but leave a breadcrumb rather than vanishing it.
+                    log::debug!("skipping non-JSON line from claude stream: {e}");
+                    continue;
+                }
             };
             // Terminal result line: { type: "result", is_error: bool,
             //                         subtype: "...", result: "..." }
@@ -1343,7 +1355,7 @@ fn spawn_claude_resume(
         let _ = BufReader::new(stderr).read_to_string(&mut stderr_buf);
 
         let status = {
-            let mut child_lock = handle.child.lock().unwrap();
+            let mut child_lock = lock_recover(&handle.child);
             child_lock.as_mut().and_then(|c| c.wait().ok())
         };
 
@@ -1372,7 +1384,7 @@ fn spawn_claude_resume(
 
         // Remove from registry on natural completion.
         let registry = app_for_thread.state::<ChildRegistry>();
-        registry.0.lock().unwrap().remove(&token_for_thread);
+        lock_recover(&registry.0).remove(&token_for_thread);
     });
 
     Ok(token)
@@ -1383,10 +1395,10 @@ fn cancel_claude_resume(
     cancel_token: String,
     registry: State<'_, ChildRegistry>,
 ) -> Result<(), String> {
-    let entry = registry.0.lock().unwrap().remove(&cancel_token);
+    let entry = lock_recover(&registry.0).remove(&cancel_token);
     if let Some(handle) = entry {
         handle.cancelled.store(true, Ordering::SeqCst);
-        if let Some(child) = handle.child.lock().unwrap().as_mut() {
+        if let Some(child) = lock_recover(&handle.child).as_mut() {
             let _ = child.kill();
         }
     }
@@ -1517,7 +1529,7 @@ pub fn run() {
                         // Buffer for cold start (frontend not yet listening) and
                         // also emit for the warm-start case where it is.
                         if let Some(pending) = handle.try_state::<PendingDeepLink>() {
-                            *pending.0.lock().unwrap() = Some(path.clone());
+                            *lock_recover(&pending.0) = Some(path.clone());
                         }
                         let _ = handle.emit("deep-link-open", path);
                     }
@@ -1769,7 +1781,7 @@ fn handle_deep_link(url: String) -> Result<Option<String>, String> {
 /// `deep-link-open` emit was dropped because no listener existed yet.
 #[tauri::command]
 fn take_pending_deep_link(pending: State<'_, PendingDeepLink>) -> Result<Option<String>, String> {
-    Ok(pending.0.lock().unwrap().take())
+    Ok(lock_recover(&pending.0).take())
 }
 
 /// Reports that a real native menu is present. The frontend uses this to yield

src/App.tsx

@@ -557,6 +557,36 @@ export default function App() {
   // the accelerators and emits an event per item; we map each to the same
   // handler the in-app shortcuts use. In a non-Tauri context (plain dev server)
   // the listeners simply never fire.
+  //
+  // The menu handlers re-create on most edits (they close over filePath,
+  // comments, etc.), so we hold the current set in a ref — refreshed every
+  // render — and register the Tauri listeners exactly once. Otherwise every
+  // keystroke would tear down and re-`listen` all of them.
+  const menuHandlersRef = useRef({
+    handleNew,
+    handleOpen,
+    handleSave,
+    handleSaveAs,
+    handleQuit,
+    handleCopyDiagnostics,
+    handleRevealLogs,
+    guardDirty,
+    openFilePath,
+    loadFileResult,
+  });
+  menuHandlersRef.current = {
+    handleNew,
+    handleOpen,
+    handleSave,
+    handleSaveAs,
+    handleQuit,
+    handleCopyDiagnostics,
+    handleRevealLogs,
+    guardDirty,
+    openFilePath,
+    loadFileResult,
+  };
+
   useEffect(() => {
     const unlisteners: (() => void)[] = [];
     (async () => {
@@ -565,24 +595,26 @@ export default function App() {
         const wire = async (event: string, fn: () => void) => {
           unlisteners.push(await listen(event, () => fn()));
         };
-        await wire('menu-new', handleNew);
-        await wire('menu-open', handleOpen);
-        await wire('menu-save', () => void handleSave());
-        await wire('menu-save-as', () => void handleSaveAs());
-        await wire('menu-quit', handleQuit);
+        const h = menuHandlersRef.current;
+        await wire('menu-new', () => h.handleNew());
+        await wire('menu-open', () => h.handleOpen());
+        await wire('menu-save', () => void h.handleSave());
+        await wire('menu-save-as', () => void h.handleSaveAs());
+        await wire('menu-quit', () => h.handleQuit());
         await wire('menu-clear-recent', () => void syncRecentMenu(clearRecentFiles()));
-        await wire('menu-copy-diagnostics', handleCopyDiagnostics);
-        await wire('menu-reveal-logs', handleRevealLogs);
+        await wire('menu-copy-diagnostics', () => void h.handleCopyDiagnostics());
+        await wire('menu-reveal-logs', () => void h.handleRevealLogs());
         // Open Recent replaces the document, so it runs through the same
         // unsaved-changes guard as File → Open and deep links.
         unlisteners.push(
           await listen<string>('menu-open-recent', (e) => {
             const path = e.payload;
             if (!path) return;
-            guardDirty(() => {
+            const cur = menuHandlersRef.current;
+            cur.guardDirty(() => {
               void (async () => {
-                const result = await openFilePath(path);
-                if (result) loadFileResult(result);
+                const result = await cur.openFilePath(path);
+                if (result) cur.loadFileResult(result);
               })();
             });
           }),
@@ -592,18 +624,8 @@ export default function App() {
       }
     })();
     return () => unlisteners.forEach((u) => u());
-  }, [
-    handleNew,
-    handleOpen,
-    handleSave,
-    handleSaveAs,
-    handleQuit,
-    handleCopyDiagnostics,
-    handleRevealLogs,
-    guardDirty,
-    openFilePath,
-    loadFileResult,
-  ]);
+    // Registered once: handlers are read live through menuHandlersRef.
+  }, []);
 
   // Fill File → Open Recent from the persisted list once on launch; after
   // this, every add/clear re-syncs the menu itself.

src/components/CommentLayer.tsx

@@ -87,34 +87,30 @@ function stackCards(cards: CardPosition[], heightFor: (id: string) => number): C
   return result;
 }
 
-function getAnchorTop(editor: Editor, commentId: string): number | null {
+// Top of an annotation's in-text highlight, in scroll-area coordinates. Comments
+// and tracked changes anchor identically — they differ only in the data
+// attribute the highlight carries.
+function getAnchorTopBy(editor: Editor, attr: 'data-comment-id' | 'data-change-id', id: string) {
   try {
-    const view = editor.view;
-    const dom = view.dom;
-    const el = dom.querySelector(`[data-comment-id="${commentId}"]`);
+    const dom = editor.view.dom;
+    const el = dom.querySelector(`[${attr}="${id}"]`);
     if (!el) return null;
     const rect = el.getBoundingClientRect();
-    const containerRect = dom.closest('.editor-scroll-area')?.getBoundingClientRect();
+    const scrollArea = dom.closest('.editor-scroll-area');
+    const containerRect = scrollArea?.getBoundingClientRect();
     if (!containerRect) return null;
-    return rect.top - containerRect.top + (dom.closest('.editor-scroll-area')?.scrollTop ?? 0);
+    return rect.top - containerRect.top + (scrollArea?.scrollTop ?? 0);
   } catch {
     return null;
   }
 }
 
+function getAnchorTop(editor: Editor, commentId: string): number | null {
+  return getAnchorTopBy(editor, 'data-comment-id', commentId);
+}
+
 function getChangeAnchorTop(editor: Editor, changeId: string): number | null {
-  try {
-    const view = editor.view;
-    const dom = view.dom;
-    const el = dom.querySelector(`[data-change-id="${changeId}"]`);
-    if (!el) return null;
-    const rect = el.getBoundingClientRect();
-    const containerRect = dom.closest('.editor-scroll-area')?.getBoundingClientRect();
-    if (!containerRect) return null;
-    return rect.top - containerRect.top + (dom.closest('.editor-scroll-area')?.scrollTop ?? 0);
-  } catch {
-    return null;
-  }
+  return getAnchorTopBy(editor, 'data-change-id', changeId);
 }
 
 export default function CommentLayer({

src/extensions/TrackChanges.ts

@@ -380,8 +380,15 @@ export function getTrackedChanges(editor: {
         });
       } else {
         const existing = changes.get(id)!;
-        existing.to = Math.max(existing.to, pos + node.nodeSize);
-        existing.text += node.text ?? '';
+        // A tracked id is minted across a single contiguous run, so the next
+        // node carrying it should abut the range we've accumulated. Extend only
+        // when it does; a non-adjacent node sharing the id would otherwise make
+        // `to` span unmarked text in between (a malformed-doc case we don't want
+        // to silently widen the range for).
+        if (pos === existing.to) {
+          existing.to = pos + node.nodeSize;
+          existing.text += node.text ?? '';
+        }
       }
     }
   });

src/hooks/useSuggestions.ts

@@ -14,26 +14,22 @@ export function useSuggestions(): UseSuggestionsReturn {
   const [suggestions, setSuggestions] = useState<Suggestion[]>([]);
 
   const acceptSuggestion = useCallback((id: string) => {
-    setSuggestions((prev) =>
-      prev.map((s) => (s.id === id ? { ...s, status: 'accepted' as const } : s)),
-    );
+    setSuggestions((prev) => prev.map((s) => (s.id === id ? { ...s, status: 'accepted' } : s)));
   }, []);
 
   const rejectSuggestion = useCallback((id: string) => {
-    setSuggestions((prev) =>
-      prev.map((s) => (s.id === id ? { ...s, status: 'rejected' as const } : s)),
-    );
+    setSuggestions((prev) => prev.map((s) => (s.id === id ? { ...s, status: 'rejected' } : s)));
   }, []);
 
   const acceptAllSuggestions = useCallback(() => {
     setSuggestions((prev) =>
-      prev.map((s) => (s.status === 'pending' ? { ...s, status: 'accepted' as const } : s)),
+      prev.map((s) => (s.status === 'pending' ? { ...s, status: 'accepted' } : s)),
     );
   }, []);
 
   const rejectAllSuggestions = useCallback(() => {
     setSuggestions((prev) =>
-      prev.map((s) => (s.status === 'pending' ? { ...s, status: 'rejected' as const } : s)),
+      prev.map((s) => (s.status === 'pending' ? { ...s, status: 'rejected' } : s)),
     );
   }, []);
 

src/test/extensions/TrackChanges.test.ts

@@ -117,6 +117,30 @@ describe('TrackChanges extension', () => {
       expect(hasMarkOfType(editor, 'tracked_insert')).toBe(false);
       expect(hasMarkOfType(editor, 'tracked_delete')).toBe(false);
     });
+
+    it('reports a single contiguous change when its run is split across text nodes', () => {
+      // Insert a tracked run, then bold part of it. Bold splits the text node in
+      // two while both halves keep the same tracked_insert id — the exact
+      // multi-node shape getTrackedChanges merges. It must still surface one
+      // change whose range spans the whole run, not two fragments.
+      editor.commands.insertContentAt(7, 'beautiful ');
+      editor.chain().setTextSelection({ from: 7, to: 11 }).toggleBold().run();
+
+      // Two adjacent text nodes now carry the run: "beau" (bold) + "tiful ".
+      const insertNodes: string[] = [];
+      editor.state.doc.descendants((node) => {
+        if (node.isText && node.marks.some((m) => m.type.name === 'tracked_insert')) {
+          insertNodes.push(node.text ?? '');
+        }
+      });
+      expect(insertNodes.length).toBeGreaterThan(1);
+
+      const changes = getTrackedChanges(editor);
+      const inserts = changes.filter((c) => c.operation === 'insert');
+      expect(inserts).toHaveLength(1);
+      expect(inserts[0].text).toBe('beautiful ');
+      expect(inserts[0].to - inserts[0].from).toBe('beautiful '.length);
+    });
   });
 
   describe('acceptChange', () => {