From 6b933d7faa51dbd42fb3335f9573414580daf5b3 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 19 Apr 2026 09:52:37 -0700 Subject: [PATCH] fix(editor): contain rich editor crashes and dedupe external reloads (#832) * fix(editor): contain TipTap render crashes and dedupe split-pane reloads Addresses issue #826 (renderer blackouts under split-pane external reload): - Wrap RichMarkdownEditor in an error boundary to contain ProseMirror transaction crashes to the affected pane. - Swallow setContent/normalizeSoftBreaks exceptions so they can't escape to the React root. - Debounce ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT dispatch per (worktreeId, relativePath) to coalesce atomic-write bursts. - Deduplicate concurrent fs/git IPC reads across split panes so a single external change doesn't fan out into N identical round-trips. * test(cli): stabilize stale bootstrap pid check --- src/cli/runtime-client.test.ts | 27 ++++- .../src/components/editor/EditorContent.tsx | 30 +++-- .../src/components/editor/EditorPanel.tsx | 107 ++++++++++++++---- .../components/editor/RichMarkdownEditor.tsx | 29 +++-- .../editor/RichMarkdownErrorBoundary.tsx | 64 +++++++++++ .../src/hooks/useEditorExternalWatch.ts | 38 ++++++- 6 files changed, 249 insertions(+), 46 deletions(-) create mode 100644 src/renderer/src/components/editor/RichMarkdownErrorBoundary.tsx diff --git a/src/cli/runtime-client.test.ts b/src/cli/runtime-client.test.ts index 6732e127a..74102e01e 100644 --- a/src/cli/runtime-client.test.ts +++ b/src/cli/runtime-client.test.ts @@ -24,12 +24,17 @@ afterEach(async () => { servers.clear() }) -function writeMetadata(userDataPath: string, endpoint: string, authToken = 'token'): void { +function writeMetadata( + userDataPath: string, + endpoint: string, + authToken = 'token', + pid = 123 +): void { writeFileSync( join(userDataPath, 'orca-runtime.json'), JSON.stringify({ runtimeId: 'runtime-1', - pid: 123, + pid, transport: { kind: 'unix', endpoint @@ -41,6 +46,22 @@ function writeMetadata(userDataPath: string, endpoint: string, authToken = 'toke ) } +function findUnusedPid(seed = 200_000): number { + // Why: the stale-bootstrap test must point metadata at a definitely-dead + // process. Hard-coding a small PID is host-dependent and flakes when that + // PID happens to be alive on the machine running the suite. + let pid = Math.max(seed, process.pid + 10_000) + while (pid < 2_000_000) { + try { + process.kill(pid, 0) + pid += 1 + } catch { + return pid + } + } + return 2_000_000 +} + // Why: these tests create Unix domain socket servers in temp directories. // Windows does not support Unix domain sockets in the same way, causing // EACCES errors on listen(), so the suite is skipped on that platform. @@ -102,7 +123,7 @@ describe.skipIf(process.platform === 'win32')('RuntimeClient', () => { it('reports stale_bootstrap when bootstrap artifacts exist but no runtime is reachable', async () => { const userDataPath = mkdtempSync(join(tmpdir(), 'orca-runtime-client-')) - writeMetadata(userDataPath, join(userDataPath, 'missing.sock')) + writeMetadata(userDataPath, join(userDataPath, 'missing.sock'), 'token', findUnusedPid()) const client = new RuntimeClient(userDataPath, 100) const status = await client.getCliStatus() diff --git a/src/renderer/src/components/editor/EditorContent.tsx b/src/renderer/src/components/editor/EditorContent.tsx index a4c56c78d..47e17ed24 100644 --- a/src/renderer/src/components/editor/EditorContent.tsx +++ b/src/renderer/src/components/editor/EditorContent.tsx @@ -8,6 +8,7 @@ import { RICH_MARKDOWN_MAX_SIZE_BYTES } from '../../../../shared/constants' import { getMarkdownRenderMode } from './markdown-render-mode' import { getMarkdownRichModeUnsupportedMessage } from './markdown-rich-mode' import { extractFrontMatter, prependFrontMatter } from './markdown-frontmatter' +import { RichMarkdownErrorBoundary } from './RichMarkdownErrorBoundary' const MonacoEditor = lazy(() => import('./MonacoEditor')) const DiffViewer = lazy(() => import('./DiffViewer')) @@ -170,18 +171,23 @@ export function EditorContent({
{fm && }
- {/* Why: same remount reasoning as MonacoEditor — see renderMonacoEditor. */} - + {/* Why: same remount reasoning as MonacoEditor — see renderMonacoEditor. + The boundary contains a TipTap/ProseMirror render crash (e.g. + when a setContent transaction throws under split-pane external + reload, issue #826) to this pane instead of letting it tear down + the whole renderer tree. */} + + +
) diff --git a/src/renderer/src/components/editor/EditorPanel.tsx b/src/renderer/src/components/editor/EditorPanel.tsx index a32e640d7..fc77f67ab 100644 --- a/src/renderer/src/components/editor/EditorPanel.tsx +++ b/src/renderer/src/components/editor/EditorPanel.tsx @@ -56,6 +56,35 @@ type FileContent = { type DiffContent = GitDiffResult +// Why: split-pane layouts mount one EditorPanel per pane, and each panel +// attaches its own listener to `ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT`. +// Without coordination, a single external write fans out into N concurrent +// `readFile` IPCs for the same path plus N independent `setContent` +// transactions on the downstream rich editors — a meaningful contributor to +// the black-window wedge reported in issue #826. Sharing a module-level +// in-flight promise per (connectionId, filePath) collapses those N reads +// into one round-trip while still letting each panel update its own local +// state with the result. +const inFlightFileReads = new Map>() +const inFlightDiffReads = new Map>() + +function inFlightReadKey(connectionId: string | undefined, filePath: string): string { + return `${connectionId ?? ''}::${filePath}` +} + +function inFlightDiffKey(file: OpenFile, connectionId: string | undefined): string { + // Why: diff content depends on the file path AND which diff source is + // being rendered (unstaged/staged/branch). Branch diffs further depend + // on the base+head oids so switching compare points doesn't alias, and + // on branchOldPath so rename-detected diffs don't alias with the same + // post-rename path viewed without rename metadata. + const branch = + file.diffSource === 'branch' && file.branchCompare + ? `${file.branchCompare.baseOid ?? ''}..${file.branchCompare.headOid ?? ''}::${file.branchOldPath ?? ''}` + : '' + return `${connectionId ?? ''}::${file.diffSource ?? ''}::${file.filePath}::${branch}` +} + function EditorPanelInner({ activeFileId: activeFileIdProp, activeViewStateId: activeViewStateIdProp @@ -215,7 +244,27 @@ function EditorPanelInner({ async (filePath: string, id: string, worktreeId?: string): Promise => { try { const connectionId = getConnectionId(worktreeId ?? null) ?? undefined - const result = (await window.api.fs.readFile({ filePath, connectionId })) as FileContent + const key = inFlightReadKey(connectionId, filePath) + // Why: share the IPC round-trip across split-pane EditorPanels viewing + // the same file. The first caller starts the read and registers the + // promise; concurrent callers (triggered by the same external-change + // event) await it instead of firing duplicate reads and duplicate + // downstream setContent transactions. + let pending = inFlightFileReads.get(key) + if (!pending) { + pending = window.api.fs.readFile({ filePath, connectionId }) as Promise + inFlightFileReads.set(key, pending) + // Why: limit deduplication to synchronous callers (like N split panes + // responding to the exact same event loop dispatch). Caching the promise + // across time (e.g. until the IPC returns) means a new change event that + // fires while the read is in-flight would receive stale content. + queueMicrotask(() => { + if (inFlightFileReads.get(key) === pending) { + inFlightFileReads.delete(key) + } + }) + } + const result = await pending setFileContents((prev) => ({ ...prev, [id]: result })) } catch (err) { setFileContents((prev) => ({ @@ -242,26 +291,42 @@ function EditorPanelInner({ ? file.branchCompare : null const connectionId = getConnectionId(file.worktreeId) ?? undefined - const result = - file.diffSource === 'branch' && branchCompare - ? ((await window.api.git.branchDiff({ - worktreePath, - compare: { - baseRef: branchCompare.baseRef, - baseOid: branchCompare.baseOid!, - headOid: branchCompare.headOid!, - mergeBase: branchCompare.mergeBase! - }, - filePath: file.relativePath, - oldPath: file.branchOldPath, - connectionId - })) as DiffContent) - : ((await window.api.git.diff({ - worktreePath, - filePath: file.relativePath, - staged: file.diffSource === 'staged', - connectionId - })) as DiffContent) + const key = inFlightDiffKey(file, connectionId) + // Why: same rationale as inFlightFileReads above — a single external + // change fans out to every mounted EditorPanel, and two split panes + // showing the same diff tab should share one git.diff IPC instead of + // racing two identical calls through the same git repo lock. + let pending = inFlightDiffReads.get(key) + if (!pending) { + pending = ( + file.diffSource === 'branch' && branchCompare + ? window.api.git.branchDiff({ + worktreePath, + compare: { + baseRef: branchCompare.baseRef, + baseOid: branchCompare.baseOid!, + headOid: branchCompare.headOid!, + mergeBase: branchCompare.mergeBase! + }, + filePath: file.relativePath, + oldPath: file.branchOldPath, + connectionId + }) + : window.api.git.diff({ + worktreePath, + filePath: file.relativePath, + staged: file.diffSource === 'staged', + connectionId + }) + ) as Promise + inFlightDiffReads.set(key, pending) + queueMicrotask(() => { + if (inFlightDiffReads.get(key) === pending) { + inFlightDiffReads.delete(key) + } + }) + } + const result = await pending setDiffContents((prev) => ({ ...prev, [file.id]: result })) } catch (err) { setDiffContents((prev) => ({ diff --git a/src/renderer/src/components/editor/RichMarkdownEditor.tsx b/src/renderer/src/components/editor/RichMarkdownEditor.tsx index 3d9ee88c7..20f3b6c0f 100644 --- a/src/renderer/src/components/editor/RichMarkdownEditor.tsx +++ b/src/renderer/src/components/editor/RichMarkdownEditor.tsx @@ -428,14 +428,27 @@ export default function RichMarkdownEditor({ // overwrite the editor state so the rich view never drifts from repo text. isApplyingProgrammaticUpdateRef.current = true try { - editor.commands.setContent(encodeRawMarkdownHtmlForRichEditor(content), { - contentType: 'markdown', - emitUpdate: false - }) - // Why: same soft-break normalization as onCreate — external content updates - // may re-introduce paragraphs with embedded `\n` characters. - normalizeSoftBreaks(editor) - lastCommittedMarkdownRef.current = content + // Why: swallow exceptions from setContent / normalizeSoftBreaks here + // rather than letting them escape to the React root. Under split-pane + // external reload (two RichMarkdownEditor instances receiving the same + // Claude Code write), a throw from the TipTap/ProseMirror transaction + // would otherwise unmount the entire renderer and black the whole + // window out (issue #826). The committed-markdown ref is deliberately + // left pointing at the pre-failure value so the next prop change still + // triggers a re-sync attempt instead of being short-circuited by the + // `content === lastCommittedMarkdownRef.current` guard above. + try { + editor.commands.setContent(encodeRawMarkdownHtmlForRichEditor(content), { + contentType: 'markdown', + emitUpdate: false + }) + // Why: same soft-break normalization as onCreate — external content updates + // may re-introduce paragraphs with embedded `\n` characters. + normalizeSoftBreaks(editor) + lastCommittedMarkdownRef.current = content + } catch (err) { + console.error('[RichMarkdownEditor] failed to apply external content update', err) + } } finally { isApplyingProgrammaticUpdateRef.current = false } diff --git a/src/renderer/src/components/editor/RichMarkdownErrorBoundary.tsx b/src/renderer/src/components/editor/RichMarkdownErrorBoundary.tsx new file mode 100644 index 000000000..b6bb74ab9 --- /dev/null +++ b/src/renderer/src/components/editor/RichMarkdownErrorBoundary.tsx @@ -0,0 +1,64 @@ +import React from 'react' + +type Props = { + fileId: string + children: React.ReactNode +} + +type State = { + error: Error | null +} + +// Why: a thrown exception inside the TipTap/ProseMirror render or in the +// effect that runs `setContent` + `normalizeSoftBreaks` on external-reload +// would escape to the React root and — without this boundary — cause React +// 18 to unmount the entire renderer subtree, blacking out the whole Orca +// window (see issue #826). Scoping the boundary to the rich-markdown editor +// contains the failure to the affected pane so the rest of the workspace +// stays usable. Re-keying on `fileId` resets the boundary when the user +// switches tabs so a transient failure doesn't permanently disable the +// rich editor for that pane. +export class RichMarkdownErrorBoundary extends React.Component { + state: State = { error: null } + + static getDerivedStateFromError(error: Error): State { + return { error } + } + + componentDidCatch(error: Error, info: React.ErrorInfo): void { + console.error('[RichMarkdownEditor] render crash contained by boundary', error, info) + } + + componentDidUpdate(prevProps: Props): void { + if (prevProps.fileId !== this.props.fileId && this.state.error) { + this.setState({ error: null }) + } + } + + handleReset = (): void => { + this.setState({ error: null }) + } + + render(): React.ReactNode { + if (this.state.error) { + return ( +
+
+ The rich markdown editor hit an unexpected error and was reset to keep the rest of Orca + responsive. +
+
+ Switch to source mode, or click retry to reload the rich view. +
+ +
+ ) + } + return this.props.children + } +} diff --git a/src/renderer/src/hooks/useEditorExternalWatch.ts b/src/renderer/src/hooks/useEditorExternalWatch.ts index c838c4af6..de0bcf38e 100644 --- a/src/renderer/src/hooks/useEditorExternalWatch.ts +++ b/src/renderer/src/hooks/useEditorExternalWatch.ts @@ -11,6 +11,34 @@ import { import type { FsChangedPayload } from '../../../shared/types' import { findWorktreeById } from '@/store/slices/worktree-helpers' +// Why: atomic-write patterns (Claude Code's Edit tool, editors like vim, +// VSCode) land as a short burst of `update` events — or `delete + create` on +// renamers — within a few milliseconds for the same path. Dispatching an +// `ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT` per raw event fan-outs into N full +// `setContent` + `normalizeSoftBreaks` doc rebuilds per mounted EditorPanel, +// which under split-pane + large markdown is enough to wedge the renderer +// and black out the window (issue #826). Coalescing per (worktreeId + path) +// on a short debounce collapses that burst into one reload notification. +const EXTERNAL_RELOAD_DEBOUNCE_MS = 75 +const pendingExternalReloadTimers = new Map() + +function scheduleDebouncedExternalReload(notification: { + worktreeId: string + worktreePath: string + relativePath: string +}): void { + const key = `${notification.worktreeId}::${notification.relativePath}` + const existing = pendingExternalReloadTimers.get(key) + if (existing !== undefined) { + window.clearTimeout(existing) + } + const handle = window.setTimeout(() => { + pendingExternalReloadTimers.delete(key) + notifyEditorExternalFileChange(notification) + }, EXTERNAL_RELOAD_DEBOUNCE_MS) + pendingExternalReloadTimers.set(key, handle) +} + type WatchedTarget = { worktreeId: string worktreePath: string @@ -180,7 +208,7 @@ export function useEditorExternalWatch(): void { // disk during the overrun stays struck through until some later // path-specific event happens to clear it. for (const notification of getOverflowExternalReloadTargets(target)) { - notifyEditorExternalFileChange(notification) + scheduleDebouncedExternalReload(notification) } // Why: `break` (not `return`) — the remaining code early-returns // when changedFiles is empty, so breaking out is semantically @@ -232,7 +260,7 @@ export function useEditorExternalWatch(): void { if (matching.some((f) => f.isDirty)) { continue } - notifyEditorExternalFileChange(notification) + scheduleDebouncedExternalReload(notification) } } @@ -250,6 +278,12 @@ export function useEditorExternalWatch(): void { }) } targetsRef.current = [] + // Why: deliberately do NOT clear pendingExternalReloadTimers here. + // The map is module-scoped, so in React StrictMode (dev) the first + // mount's cleanup would otherwise drop timers scheduled by the second + // mount. A late `notifyEditorExternalFileChange` dispatch after unmount + // is also harmless — it's a window event with no EditorPanel listeners + // attached once the editor tree is torn down. } }, []) }