From b81492f5ae6998acfcf2336bec5f547e64efceac Mon Sep 17 00:00:00 2001 From: Sam Powers <35611153+sam-powers@users.noreply.github.com> Date: Sun, 14 Jun 2026 08:06:54 -0400 Subject: [PATCH] chore: code-review fixes (menu churn, split-mark range, mutex recovery) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A code-quality sweep after the security hardening pass. Three substantive fixes plus cosmetic cleanups: - App.tsx: register the Tauri menu-event listeners once instead of re-listening on every edit. The handlers close over doc state and re-create on most keystrokes, which tore down and re-`listen`ed all of them each render. Hold them in a ref refreshed per render; the effect deps are now empty. - TrackChanges.ts (getTrackedChanges): only extend a tracked change's range when the next node carrying its id actually abuts the accumulated range. The old unconditional Math.max would, on a malformed doc with a non-adjacent node sharing an id, widen `to` across unmarked text. Adds a test covering the split-across-text-nodes (bold inside a tracked run) merge case. - lib.rs: route the six ChildRegistry / PendingDeepLink / child-handle mutex locks through a `lock_recover` helper that recovers the inner guard on poisoning instead of panicking — these guard plain data that stays valid across a panic. Also log (debug) skipped non-JSON lines in the claude stream rather than dropping them silently. Cosmetic: dedupe the two identical anchor-lookup helpers in CommentLayer.tsx; drop redundant `as const` in useSuggestions.ts. Co-Authored-By: Claude Opus 4.8 --- src-tauri/src/lib.rs | 36 ++++++++----- src/App.tsx | 66 ++++++++++++++++-------- src/components/CommentLayer.tsx | 32 +++++------- src/extensions/TrackChanges.ts | 11 +++- src/hooks/useSuggestions.ts | 12 ++--- src/test/extensions/TrackChanges.test.ts | 24 +++++++++ 6 files changed, 119 insertions(+), 62 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 2b76883..83702b1 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -613,6 +613,17 @@ struct ChildRegistry(Mutex>>); #[derive(Default)] struct PendingDeepLink(Mutex>); +/// 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(mutex: &Mutex) -> std::sync::MutexGuard<'_, T> { + mutex + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} + fn claude_projects_dir() -> Result { 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::(); - registry - .0 - .lock() - .unwrap() - .insert(token.clone(), handle.clone()); + lock_recover(®istry.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::(); - registry.0.lock().unwrap().remove(&token_for_thread); + lock_recover(®istry.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(®istry.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::() { - *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, String> { /// `deep-link-open` emit was dropped because no listener existed yet. #[tauri::command] fn take_pending_deep_link(pending: State<'_, PendingDeepLink>) -> Result, 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 diff --git a/src/App.tsx b/src/App.tsx index af49a24..bcb61dc 100644 --- a/src/App.tsx +++ b/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('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. diff --git a/src/components/CommentLayer.tsx b/src/components/CommentLayer.tsx index a39d03c..56dcb3a 100644 --- a/src/components/CommentLayer.tsx +++ b/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({ diff --git a/src/extensions/TrackChanges.ts b/src/extensions/TrackChanges.ts index 88006d8..a3c8277 100644 --- a/src/extensions/TrackChanges.ts +++ b/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 ?? ''; + } } } }); diff --git a/src/hooks/useSuggestions.ts b/src/hooks/useSuggestions.ts index ac7ec11..ca9b1de 100644 --- a/src/hooks/useSuggestions.ts +++ b/src/hooks/useSuggestions.ts @@ -14,26 +14,22 @@ export function useSuggestions(): UseSuggestionsReturn { const [suggestions, setSuggestions] = useState([]); 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)), ); }, []); diff --git a/src/test/extensions/TrackChanges.test.ts b/src/test/extensions/TrackChanges.test.ts index 31610bc..a930543 100644 --- a/src/test/extensions/TrackChanges.test.ts +++ b/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', () => {