mirror of
https://github.com/stablyai/orca.git
synced 2026-10-05 08:02:33 +00:00
fix(editor): keep the unparseable editor path visible after the URI guard
Round-1 review remediation.
Blocking: the guard turned the only observable instance of an unidentified
input class into a silent rewrite plus a silent catch. Both paths now leave a
shape-only breadcrumb through the existing `recordRendererCrashBreadcrumb`
leaf API, so the class still lands in the crash report's "Recent activity"
block:
- `editor_model_path_uri_rejected` from the fallback rewrite
- `editor_model_dispose_path_unkeyable` from the disposal catch
Fields are length, colon count, has-backslash, and the character classes of
the scheme prefix — never the path itself (a test asserts the payload cannot
contain the host name).
Also fixed:
- The disposal catch's justification comment was stale. It argued no model
could leak because @monaco-editor/react keyed with the identical parse;
Part 2 makes exactly those paths produce a model. It now says what the
branch actually guards: `Uri.file()` itself rejecting the path.
- Test coupling gap: MonacoEditor.model-path.test.tsx now feeds the `path`
prop Monaco actually received into `disposeClosedEditorTabs`, so create/
dispose key agreement fails if MonacoEditor stops using the helper. The
fake registry moved to monaco-model-registry-test-fixture.ts instead of
being duplicated.
- Windows form pinned: monaco-edit-model-path.windows.test.ts pins
`process.platform` before monaco loads and asserts the exact UNC-aware
fallback (`file://wsl.localhost/.../notes%3A2026.md`) real users get, plus
that it stays a stable dispose key.
- `ClosedEditorTabMonacoRegistry` uses `Omit<MonacoModelRegistry, 'Uri'>`
rather than intersecting a second `Uri` onto a type that declares one, and
is exported so tests stop redeclaring it. Widening
`MonacoModelRegistry['Uri']` in place was rejected: diff disposal never
calls `Uri.file`, so that would force unused members onto its fakes.
Correction to the previous write-up, not the code: "that is the producer: a
WSL/Linux-produced name joined onto a `\\wsl.localhost` Windows root" was
stated as fact and is an inference. The probe shows only that WSL-side git can
emit an ASCII colon, and it refutes the Win32-enumeration route (U+F03A does
not throw). The honest claim is: mechanism narrowed, producing string not
recovered — which is exactly why the breadcrumb above is needed.
Verification: 199 files / 1331 tests pass in the editor folder; typecheck exit
0; oxlint on the editor folder exit 0; oxfmt clean; changed-code quality gate
reports 0 new findings across 8 files. RED confirmed for the new crumb by
removing the fallback record call (1 failed).
This commit is contained in:
@@ -2,6 +2,9 @@
|
||||
import { cleanup, render } from '@testing-library/react'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { Uri } from 'monaco-editor'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import { disposeClosedEditorTabs } from './closed-editor-tab-disposal'
|
||||
import { createMonacoModelRegistryWithRealUri } from './monaco-model-registry-test-fixture'
|
||||
|
||||
const editorProps = vi.hoisted(() => ({ current: null as Record<string, unknown> | null }))
|
||||
|
||||
@@ -72,4 +75,18 @@ describe('MonacoEditor model path', () => {
|
||||
expect(() => renderEditor(WSL_COLON_PATH)).not.toThrow()
|
||||
expect(() => Uri.parse(String(editorProps.current?.path))).not.toThrow()
|
||||
})
|
||||
|
||||
// Why through the rendered prop, not the helper: this is the only assertion that fails if
|
||||
// MonacoEditor stops routing `path` through the helper and the close-all key drifts.
|
||||
it('keys the model on a path close-all can still dispose', () => {
|
||||
renderEditor(WSL_COLON_PATH)
|
||||
const renderedPath = String(editorProps.current?.path)
|
||||
const registry = createMonacoModelRegistryWithRealUri([renderedPath])
|
||||
|
||||
disposeClosedEditorTabs(registry, [
|
||||
{ id: 'poisoned', mode: 'edit', filePath: WSL_COLON_PATH } as OpenFile
|
||||
])
|
||||
|
||||
expect(registry.disposed).toEqual([renderedPath])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -6,13 +6,14 @@ import {
|
||||
scrollTopCache
|
||||
} from '@/lib/scroll-cache'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import { disposeClosedEditorTabs } from './closed-editor-tab-disposal'
|
||||
import {
|
||||
disposeClosedEditorTabs,
|
||||
type ClosedEditorTabMonacoRegistry
|
||||
} from './closed-editor-tab-disposal'
|
||||
import {
|
||||
getDiffViewerMonacoModelPaths,
|
||||
getDiffViewerMonacoModelPathPrefixes,
|
||||
type MonacoModelRegistry
|
||||
getDiffViewerMonacoModelPathPrefixes
|
||||
} from './diff-monaco-model-disposal'
|
||||
import type { MonacoUriNamespace } from './monaco-edit-model-path'
|
||||
|
||||
const CLOSED_DIFF_TAB_COUNT = 100
|
||||
const RETAINED_MODEL_COUNT = 320
|
||||
@@ -26,8 +27,7 @@ type FakeModel = {
|
||||
uri: { toString: (skipEncoding?: boolean) => string }
|
||||
}
|
||||
|
||||
type FakeRegistry = MonacoModelRegistry & {
|
||||
Uri: MonacoUriNamespace
|
||||
type FakeRegistry = ClosedEditorTabMonacoRegistry & {
|
||||
models: FakeModel[]
|
||||
counters: { getModelsCalls: number; uriToStringCalls: number }
|
||||
}
|
||||
|
||||
@@ -14,9 +14,15 @@ import {
|
||||
deletePaneScopedCacheEntries,
|
||||
sweepClosedPdfViewPositions
|
||||
} from './closed-editor-tab-cache-sweep'
|
||||
import { toMonacoEditModelPath, type MonacoUriNamespace } from './monaco-edit-model-path'
|
||||
import {
|
||||
recordUnparseableModelPathShape,
|
||||
toMonacoEditModelPath,
|
||||
type MonacoUriNamespace
|
||||
} from './monaco-edit-model-path'
|
||||
|
||||
type ClosedEditorTabMonacoRegistry = MonacoModelRegistry & { Uri: MonacoUriNamespace }
|
||||
export type ClosedEditorTabMonacoRegistry = Omit<MonacoModelRegistry, 'Uri'> & {
|
||||
Uri: MonacoUriNamespace
|
||||
}
|
||||
|
||||
function disposeEditTabModel(
|
||||
monacoRegistry: ClosedEditorTabMonacoRegistry,
|
||||
@@ -26,7 +32,8 @@ function disposeEditTabModel(
|
||||
try {
|
||||
modelUri = monacoRegistry.Uri.parse(toMonacoEditModelPath(monacoRegistry.Uri, filePath))
|
||||
} catch {
|
||||
// Why safe: @monaco-editor/react keys the model with this identical parse, so a path that throws here never produced a model to leak.
|
||||
// Reached only if Uri.file() itself rejects the path: no key exists, so nothing to dispose.
|
||||
recordUnparseableModelPathShape('editor_model_dispose_path_unkeyable', filePath)
|
||||
return
|
||||
}
|
||||
monacoRegistry.editor.getModel(modelUri)?.dispose()
|
||||
|
||||
@@ -1,11 +1,22 @@
|
||||
// @vitest-environment happy-dom
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { Uri } from 'monaco-editor'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import { disposeClosedEditorTabs } from './closed-editor-tab-disposal'
|
||||
import { toMonacoEditModelPath } from './monaco-edit-model-path'
|
||||
import type { MonacoModelRegistry } from './diff-monaco-model-disposal'
|
||||
import type { MonacoUriNamespace } from './monaco-edit-model-path'
|
||||
import {
|
||||
createMonacoModelRegistryWithRealUri,
|
||||
type RecordingMonacoModelRegistry
|
||||
} from './monaco-model-registry-test-fixture'
|
||||
|
||||
const crashBreadcrumbs = vi.hoisted(() => ({ record: vi.fn() }))
|
||||
vi.mock('@/lib/crash-breadcrumb-recorder', () => ({
|
||||
recordRendererCrashBreadcrumb: crashBreadcrumbs.record
|
||||
}))
|
||||
|
||||
beforeEach(() => {
|
||||
crashBreadcrumbs.record.mockClear()
|
||||
})
|
||||
|
||||
// Why this exact shape: the field crash (`[UriError]: Scheme contains illegal characters.`) came
|
||||
// from a Windows/WSL workspace. `Uri.parse` reads `\\wsl.localhost\...\notes` as the scheme
|
||||
@@ -13,42 +24,6 @@ import type { MonacoUriNamespace } from './monaco-edit-model-path'
|
||||
const WSL_COLON_PATH =
|
||||
'\\\\wsl.localhost\\Ubuntu-26.04\\home\\mj\\projects\\acp-client\\notes:2026.md'
|
||||
|
||||
type RecordingRegistry = MonacoModelRegistry & {
|
||||
Uri: MonacoUriNamespace
|
||||
disposed: string[]
|
||||
}
|
||||
|
||||
function createRegistryWithRealUri(modelPaths: readonly string[]): RecordingRegistry {
|
||||
const disposed: string[] = []
|
||||
const modelsByUri = new Map<string, ReturnType<typeof createModel>>()
|
||||
|
||||
function createModel(modelPath: string): {
|
||||
dispose: () => void
|
||||
isAttachedToEditor: () => boolean
|
||||
uri: { toString: (skipEncoding?: boolean) => string }
|
||||
} {
|
||||
const key = Uri.parse(modelPath).toString()
|
||||
return {
|
||||
dispose: () => disposed.push(modelPath),
|
||||
isAttachedToEditor: () => false,
|
||||
uri: { toString: () => key }
|
||||
}
|
||||
}
|
||||
|
||||
for (const modelPath of modelPaths) {
|
||||
modelsByUri.set(Uri.parse(modelPath).toString(), createModel(modelPath))
|
||||
}
|
||||
|
||||
return {
|
||||
disposed,
|
||||
Uri,
|
||||
editor: {
|
||||
getModel: (uri: unknown) => modelsByUri.get(String(uri)) ?? null,
|
||||
getModels: () => [...modelsByUri.values()]
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
function editTab(id: string, filePath: string): OpenFile {
|
||||
return { id, mode: 'edit', filePath } as OpenFile
|
||||
}
|
||||
@@ -71,11 +46,32 @@ describe('toMonacoEditModelPath', () => {
|
||||
expect(modelPath).not.toBe(WSL_COLON_PATH)
|
||||
expect(() => Uri.parse(modelPath)).not.toThrow()
|
||||
})
|
||||
|
||||
// Why: the rewrite is now the only trace of an input class whose producer is unidentified.
|
||||
it('leaves a shape-only crumb describing the rejected path', () => {
|
||||
toMonacoEditModelPath(Uri, WSL_COLON_PATH)
|
||||
|
||||
expect(crashBreadcrumbs.record).toHaveBeenCalledWith('editor_model_path_uri_rejected', {
|
||||
length: WSL_COLON_PATH.length,
|
||||
colons: 1,
|
||||
hasBackslash: true,
|
||||
schemePrefixLength: WSL_COLON_PATH.indexOf(':'),
|
||||
schemePrefixCharset: 'alpha|backslash|digit|schemeSafePunct'
|
||||
})
|
||||
const [, data] = crashBreadcrumbs.record.mock.calls[0] as [string, Record<string, unknown>]
|
||||
expect(JSON.stringify(data)).not.toContain('wsl.localhost')
|
||||
})
|
||||
|
||||
it('records nothing for a path Monaco already accepts', () => {
|
||||
toMonacoEditModelPath(Uri, '/repo/file.py')
|
||||
|
||||
expect(crashBreadcrumbs.record).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('disposeClosedEditorTabs with the real Monaco URI parser', () => {
|
||||
it('does not throw the workbench down on the unparseable path class', () => {
|
||||
const registry = createRegistryWithRealUri([])
|
||||
const registry = createMonacoModelRegistryWithRealUri([])
|
||||
|
||||
expect(() =>
|
||||
disposeClosedEditorTabs(registry, [editTab('poisoned', WSL_COLON_PATH)])
|
||||
@@ -85,7 +81,7 @@ describe('disposeClosedEditorTabs with the real Monaco URI parser', () => {
|
||||
it('keeps disposing the rest of the close-all batch behind a poisoned tab', () => {
|
||||
const beforePath = '/repo/before.ts'
|
||||
const afterPath = '/repo/after.ts'
|
||||
const registry = createRegistryWithRealUri([
|
||||
const registry = createMonacoModelRegistryWithRealUri([
|
||||
beforePath,
|
||||
toMonacoEditModelPath(Uri, WSL_COLON_PATH),
|
||||
afterPath
|
||||
@@ -102,4 +98,24 @@ describe('disposeClosedEditorTabs with the real Monaco URI parser', () => {
|
||||
// Why: the poisoned tab's model is reachable too, because open and close now key it the same way.
|
||||
expect(registry.disposed).toContain(toMonacoEditModelPath(Uri, WSL_COLON_PATH))
|
||||
})
|
||||
|
||||
it('crumbs instead of throwing when even the fallback URI form is unbuildable', () => {
|
||||
const registry: RecordingMonacoModelRegistry = {
|
||||
...createMonacoModelRegistryWithRealUri([]),
|
||||
Uri: {
|
||||
parse: (value: string) => Uri.parse(value),
|
||||
file: () => {
|
||||
throw new Error('unbuildable')
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
expect(() =>
|
||||
disposeClosedEditorTabs(registry, [editTab('poisoned', WSL_COLON_PATH)])
|
||||
).not.toThrow()
|
||||
expect(crashBreadcrumbs.record).toHaveBeenCalledWith(
|
||||
'editor_model_dispose_path_unkeyable',
|
||||
expect.objectContaining({ hasBackslash: true })
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,8 +1,48 @@
|
||||
import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder'
|
||||
|
||||
export type MonacoUriNamespace = {
|
||||
parse(value: string): unknown
|
||||
file(path: string): { toString(): string }
|
||||
}
|
||||
|
||||
const SCHEME_PREFIX_CHAR_CLASSES: readonly (readonly [string, RegExp])[] = [
|
||||
['alpha', /[a-z]/i],
|
||||
['digit', /\d/],
|
||||
['backslash', /\\/],
|
||||
['slash', /\//],
|
||||
['schemeSafePunct', /[.+-]/],
|
||||
['space', / /]
|
||||
]
|
||||
|
||||
/** Character classes before the first `:`, so a crumb names the shape without carrying the path. */
|
||||
function schemePrefixCharset(prefix: string): string {
|
||||
const classes = new Set(
|
||||
[...prefix].map(
|
||||
(char) => SCHEME_PREFIX_CHAR_CLASSES.find(([, pattern]) => pattern.test(char))?.[0] ?? 'other'
|
||||
)
|
||||
)
|
||||
return [...classes].sort().join('|')
|
||||
}
|
||||
|
||||
/**
|
||||
* Shape-only crumb for a path Monaco's URI parser rejects.
|
||||
*
|
||||
* Why at all: the guards below turn the field crash into a silent rewrite, and the crash was the
|
||||
* only signal that ever surfaced this input class — its producer is still unidentified. Shape
|
||||
* fields only; the crash pipeline redacts raw paths anyway.
|
||||
*/
|
||||
export function recordUnparseableModelPathShape(name: string, filePath: string): void {
|
||||
const path = String(filePath)
|
||||
const firstColon = path.indexOf(':')
|
||||
recordRendererCrashBreadcrumb(name, {
|
||||
length: path.length,
|
||||
colons: path.split(':').length - 1,
|
||||
hasBackslash: path.includes('\\'),
|
||||
schemePrefixLength: firstColon,
|
||||
schemePrefixCharset: schemePrefixCharset(firstColon === -1 ? '' : path.slice(0, firstColon))
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* The path an edit tab's Monaco model is keyed by, in a form `Uri.parse` always accepts.
|
||||
*
|
||||
@@ -17,6 +57,7 @@ export function toMonacoEditModelPath(uri: MonacoUriNamespace, filePath: string)
|
||||
uri.parse(filePath)
|
||||
return filePath
|
||||
} catch {
|
||||
recordUnparseableModelPathShape('editor_model_path_uri_rejected', filePath)
|
||||
return uri.file(filePath).toString()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
// @vitest-environment happy-dom
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
|
||||
// Why a dedicated file: monaco reads `process.platform` once at module load, so the Windows URI
|
||||
// form — the one real users of this crash get — is only reachable by pinning it before the import.
|
||||
vi.hoisted(() => {
|
||||
Object.defineProperty(process, 'platform', { value: 'win32', configurable: true })
|
||||
})
|
||||
|
||||
import { Uri } from 'monaco-editor'
|
||||
import { isWindows } from 'monaco-editor/esm/vs/base/common/platform.js'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import { disposeClosedEditorTabs } from './closed-editor-tab-disposal'
|
||||
import { toMonacoEditModelPath } from './monaco-edit-model-path'
|
||||
import { createMonacoModelRegistryWithRealUri } from './monaco-model-registry-test-fixture'
|
||||
|
||||
const WSL_COLON_PATH =
|
||||
'\\\\wsl.localhost\\Ubuntu-26.04\\home\\mj\\projects\\acp-client\\notes:2026.md'
|
||||
|
||||
describe('toMonacoEditModelPath on Windows', () => {
|
||||
it('pins the UNC-aware fallback form monaco builds when the renderer is Windows', () => {
|
||||
expect(isWindows).toBe(true)
|
||||
expect(toMonacoEditModelPath(Uri, WSL_COLON_PATH)).toBe(
|
||||
'file://wsl.localhost/Ubuntu-26.04/home/mj/projects/acp-client/notes%3A2026.md'
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps the Windows form a stable dispose key', () => {
|
||||
const modelPath = toMonacoEditModelPath(Uri, WSL_COLON_PATH)
|
||||
const registry = createMonacoModelRegistryWithRealUri([modelPath])
|
||||
|
||||
disposeClosedEditorTabs(registry, [
|
||||
{ id: 'poisoned', mode: 'edit', filePath: WSL_COLON_PATH } as OpenFile
|
||||
])
|
||||
|
||||
expect(registry.disposed).toEqual([modelPath])
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,44 @@
|
||||
import { Uri } from 'monaco-editor'
|
||||
import type { ClosedEditorTabMonacoRegistry } from './closed-editor-tab-disposal'
|
||||
|
||||
export type RecordingMonacoModelRegistry = ClosedEditorTabMonacoRegistry & {
|
||||
disposed: string[]
|
||||
}
|
||||
|
||||
/**
|
||||
* A model registry backed by the real Monaco URI parser.
|
||||
*
|
||||
* Why real: the whole point of these tests is which paths `Uri.parse` accepts, so a fake parser
|
||||
* that echoes its input would assert nothing.
|
||||
*/
|
||||
export function createMonacoModelRegistryWithRealUri(
|
||||
modelPaths: readonly string[]
|
||||
): RecordingMonacoModelRegistry {
|
||||
const disposed: string[] = []
|
||||
const modelsByUri = new Map<
|
||||
string,
|
||||
{
|
||||
dispose: () => void
|
||||
isAttachedToEditor: () => boolean
|
||||
uri: { toString: (skipEncoding?: boolean) => string }
|
||||
}
|
||||
>()
|
||||
|
||||
for (const modelPath of modelPaths) {
|
||||
const key = Uri.parse(modelPath).toString()
|
||||
modelsByUri.set(key, {
|
||||
dispose: () => disposed.push(modelPath),
|
||||
isAttachedToEditor: () => false,
|
||||
uri: { toString: () => key }
|
||||
})
|
||||
}
|
||||
|
||||
return {
|
||||
disposed,
|
||||
Uri,
|
||||
editor: {
|
||||
getModel: (uri: unknown) => modelsByUri.get(String(uri)) ?? null,
|
||||
getModels: () => [...modelsByUri.values()]
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user