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
This commit is contained in:
Jinjing
GitHub
parent
82bbc48932
commit
6b933d7faa
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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({
|
|||
<div className="flex h-full min-h-0 flex-col">
|
||||
{fm && <FrontMatterBanner raw={fm.raw} />}
|
||||
<div className="min-h-0 flex-1">
|
||||
{/* Why: same remount reasoning as MonacoEditor — see renderMonacoEditor. */}
|
||||
<RichMarkdownEditor
|
||||
key={viewStateScopeId}
|
||||
fileId={activeFile.id}
|
||||
content={editorContent}
|
||||
filePath={activeFile.filePath}
|
||||
worktreeId={activeFile.worktreeId}
|
||||
scrollCacheKey={`${editorViewStateKey}:rich`}
|
||||
onContentChange={onContentChangeWithFm}
|
||||
onDirtyStateHint={handleDirtyStateHint}
|
||||
onSave={onSaveWithFm}
|
||||
/>
|
||||
{/* 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. */}
|
||||
<RichMarkdownErrorBoundary key={viewStateScopeId} fileId={activeFile.id}>
|
||||
<RichMarkdownEditor
|
||||
fileId={activeFile.id}
|
||||
content={editorContent}
|
||||
filePath={activeFile.filePath}
|
||||
worktreeId={activeFile.worktreeId}
|
||||
scrollCacheKey={`${editorViewStateKey}:rich`}
|
||||
onContentChange={onContentChangeWithFm}
|
||||
onDirtyStateHint={handleDirtyStateHint}
|
||||
onSave={onSaveWithFm}
|
||||
/>
|
||||
</RichMarkdownErrorBoundary>
|
||||
</div>
|
||||
</div>
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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<string, Promise<FileContent>>()
|
||||
const inFlightDiffReads = new Map<string, Promise<DiffContent>>()
|
||||
|
||||
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<void> => {
|
||||
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<FileContent>
|
||||
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<DiffContent>
|
||||
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) => ({
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Props, State> {
|
||||
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 (
|
||||
<div className="flex h-full min-h-0 flex-col items-center justify-center gap-3 px-6 text-center text-sm text-muted-foreground">
|
||||
<div>
|
||||
The rich markdown editor hit an unexpected error and was reset to keep the rest of Orca
|
||||
responsive.
|
||||
</div>
|
||||
<div className="text-xs opacity-70">
|
||||
Switch to source mode, or click retry to reload the rich view.
|
||||
</div>
|
||||
<button
|
||||
className="rounded border border-border/60 px-3 py-1 text-xs hover:bg-accent"
|
||||
onClick={this.handleReset}
|
||||
>
|
||||
Retry
|
||||
</button>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
return this.props.children
|
||||
}
|
||||
}
|
||||
|
|
@ -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<string, number>()
|
||||
|
||||
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.
|
||||
}
|
||||
}, [])
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue