Elgato_dark/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca synced 2026-04-21 14:17:16 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

test(e2e): key Monaco registry by viewStateKey to fix split-pane lifecycle

Browse source 
Two split panes viewing the same file are distinct <MonacoEditor> React
instances with distinct viewStateKey values. Keying the dev/E2E registry
by filePath made the second mount overwrite the first; because cleanup is
guarded by editorRef identity, the first pane's later unmount couldn't
find a matching entry and left the registry empty while a live pane still
existed. Keying by viewStateKey gives each pane its own slot so set and
delete share the same unique key.

Also:
- Declare window.__monacoEditors on Window in env.d.ts and
  helpers/runtime-types.ts so reads/writes are fully typed in both
  renderer and test harness.
- getMonacoContent now scans the registry by model URI fsPath so specs
  that don't care about split panes can look up "the editor showing this
  file" without knowing its viewStateKey.
- Restore markdownViewMode's `?? 'rich'` default at the test read site to
  match EditorPanel.tsx's effective mode — the store map is undefined for
  files the user never explicitly toggled.
This commit is contained in:
brennanb2025 2026-04-19 17:45:28 -07:00
parent 5ab5f3d0c2
commit 4c8603b523
5 changed files with 165 additions and 54 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

41
src/renderer/src/components/editor/MonacoEditor.tsx
View file

@ -163,18 +163,25 @@ export default function MonacoEditor({
(editorInstance, monaco) => {
editorRef.current = editorInstance
// Why: expose the live Monaco editor instance by filePath in dev/E2E so
// tests can assert on model content via the public `editor.getValue()`
// API instead of scraping the internal `.view-lines` DOM — which has no
// SemVer contract and could break on Monaco upgrades. Same gating as
// Why: expose the live Monaco editor instance in dev/E2E so tests can
// assert on model content via the public `editor.getValue()` API instead
// of scraping the internal `.view-lines` DOM — which has no SemVer
// contract and could break on Monaco upgrades. Same gating as
// window.__store (see store/index.ts) so production builds are unaffected.
//
// Why key on viewStateKey (per-pane) rather than filePath: EditorContent
// mounts one <MonacoEditor> per visible pane with `key={viewStateScopeId}`
// so two split panes viewing the same file are distinct React instances
// with distinct viewStateKey values. Keying on filePath would collide
// (second mount overwrites the first), and because cleanup below is
// guarded by editorRef identity, the first pane's later unmount would
// find a non-matching entry and leave the registry empty while a live
// pane still exists. Using viewStateKey makes set and delete share the
// same unique per-pane slot, eliminating that asymmetric-lifecycle bug.
if ((import.meta.env.DEV || e2eConfig.exposeStore) && typeof window !== 'undefined') {
const registry =
((window as unknown as Record<string, unknown>).__monacoEditors as
| Map<string, editor.IStandaloneCodeEditor>
| undefined) ?? new Map<string, editor.IStandaloneCodeEditor>()
registry.set(filePath, editorInstance)
;(window as unknown as Record<string, unknown>).__monacoEditors = registry
const registry = window.__monacoEditors ?? new Map<string, editor.IStandaloneCodeEditor>()
registry.set(viewStateKey, editorInstance)
window.__monacoEditors = registry
}
// Why: see comment on contentRef — reconcile the retained model against
@ -350,17 +357,17 @@ export default function MonacoEditor({
clearTransientRevealHighlight()
// Drop the dev/E2E editor registry entry so stale disposed instances
// can't leak to tests that run after this tab unmounts.
// can't leak to tests that run after this tab unmounts. Keyed by
// viewStateKey (per-pane, unique even across split panes viewing the
// same file) so set and delete share the same slot — see handleMount.
if ((import.meta.env.DEV || e2eConfig.exposeStore) && typeof window !== 'undefined') {
const registry = (window as unknown as Record<string, unknown>).__monacoEditors as
| Map<string, editor.IStandaloneCodeEditor>
| undefined
if (registry?.get(filePath) === editorRef.current) {
registry.delete(filePath)
const registry = window.__monacoEditors
if (registry?.get(viewStateKey) === editorRef.current) {
registry.delete(viewStateKey)
}
}
}
}, [cancelScheduledReveal, clearTransientRevealHighlight, viewStateKey, filePath])
}, [cancelScheduledReveal, clearTransientRevealHighlight, viewStateKey])
// Update editor options when settings change
useEffect(() => {

8
src/renderer/src/env.d.ts vendored
View file

@ -1,5 +1,6 @@
/// <reference types="vite/client" />
import type { editor } from 'monaco-editor'
import type { PaneManager } from '@/lib/pane-manager/pane-manager'
declare global {
@ -11,6 +12,13 @@ declare global {
// oxlint-disable-next-line typescript-eslint/consistent-type-definitions -- declaration merging requires interface
interface Window {
__paneManagers?: Map<string, PaneManager>
// Why: MonacoEditor.tsx exposes the live editor instances in dev/E2E keyed
// by viewStateKey (one entry per mounted pane, unique even when split panes
// view the same file). Declared here so the renderer can read/write the
// registry without casting through `unknown`, and so a future shape change
// is caught at compile time. The test harness mirrors this augmentation in
// tests/e2e/helpers/runtime-types.ts for its own tsconfig.
__monacoEditors?: Map<string, editor.IStandaloneCodeEditor>
}
}

132
tests/e2e/external-file-change.spec.ts
View file

@ -163,8 +163,12 @@ test.describe('External File Change Reflection', () => {
expect(fileId).not.toBeNull()
await activateEditorTab(orcaPage, fileId!)
// Confirm the tab entered rich mode before the external write. Without
// this baseline, "still rich after write" could mean "never was rich."
// Confirm the tab is in rich mode before the external write. Why the
// `?? 'rich'` default: the store's `markdownViewMode` map is populated
// only when the user explicitly toggles to source mode — EditorPanel.tsx
// applies the `'rich'` default at the read site. We mirror that here so
// this assertion matches the effective mode the user sees, not the raw
// map (which is `undefined` for a file that was never toggled).
await expect
.poll(
async () =>
@ -198,9 +202,12 @@ test.describe('External File Change Reflection', () => {
return null
}
const file = state.openFiles.find((f) => f.id === id)
if (!file) {
return null
}
return {
isDirty: Boolean(file?.isDirty),
externalMutation: file?.externalMutation ?? null,
isDirty: Boolean(file.isDirty),
externalMutation: file.externalMutation ?? null,
mode: state.markdownViewMode[id] ?? 'rich'
}
}, fileId!),
@ -273,34 +280,91 @@ test.describe('External File Change Reflection', () => {
.poll(async () => getOpenFileSummary(orcaPage, fileId!), { timeout: 3_000 })
.toEqual({ isDirty: true, externalMutation: null, draft: draftContent })
// External writer stomps on the same path.
const externalToken = `external-clobber-attempt-${Date.now()}`
const externalContent = `export const scratch = "${externalToken}"\n`
writeFileSync(scratchAbs, externalContent)
// Why a second "sibling" scratch file: we need a positive signal that
// the external-reload pipeline has actually fired and settled, rather
// than sleeping a fixed 500ms and hoping. By opening a clean sibling
// tab, writing both externals back-to-back, and waiting until the
// sibling's Monaco model reflects its external content, we prove the
// reload pipeline has run end-to-end under identical timing. Only
// then do we assert the dirty-guard skipped the dirty tab.
const siblingRel = `scratch-external-dirty-sibling-${Date.now()}-${Math.random().toString(36).slice(2, 8)}.ts`
const siblingAbs = path.join(worktreePath!, siblingRel)
const siblingSeed = 'export const sibling = "initial"\n'
// The guard: the draft must survive, the dirty flag must stay, and we
// must NOT have been marked as tombstoned/renamed. The reload pipeline
// in PR #735 skips the scheduleDebouncedExternalReload call when any
// matching openFile is dirty — verifying the draft didn't shift is the
// strongest store-visible proof that skip happened.
//
// Wait long enough to outlast the 75ms debounce + fs event settle, so
// a regression that *did* reload would have observably clobbered by now.
await orcaPage.waitForTimeout(500)
const afterWrite = await getOpenFileSummary(orcaPage, fileId!)
expect(afterWrite).toEqual({
isDirty: true,
externalMutation: null,
draft: draftContent
})
try {
// Write the sibling seed inside the try so the finally still cleans
// up if anything between here and the sibling-open throws.
writeFileSync(siblingAbs, siblingSeed)
// The load-bearing assertion for PR #735's dirty-guard: Monaco's model
// must NOT carry the external token. A regression that unconditionally
// reloaded on external writes would paint the external content over the
// user's in-progress edit, silently losing their work — exactly the
// failure mode the guard was added to prevent.
const monacoContent = await getMonacoContent(orcaPage, scratchAbs)
expect(monacoContent).not.toContain(externalToken)
const siblingId = await openFileInStore(orcaPage, worktreeId, siblingRel)
expect(siblingId).not.toBeNull()
// Leave the sibling as the active tab. Orca's EditorPanel only mounts
// a MonacoEditor for the ACTIVE file, so we need the sibling active
// for its Monaco model to exist (and thus for getMonacoContent to
// return a non-null value usable as our positive signal). The
// dirty-guard in PR #735 is path-scoped — it reads openFiles from
// the store and matches by path, skipping the reload for any
// matching openFile that is dirty — so the guard still fires for
// the (now-inactive) dirty tab when the external write lands. See
// src/renderer/src/hooks/useEditorExternalWatch.ts around lines
// 377-391.
// Monaco must mount the sibling with its seed before the external
// write, or the post-write poll could race a still-mounting pane
// and see its external content only because it rendered late.
await expect
.poll(async () => getMonacoContent(orcaPage, siblingAbs), { timeout: 10_000 })
.toBe(siblingSeed)
// External writer stomps both paths in the same moment.
const externalToken = `external-clobber-attempt-${Date.now()}`
const externalContent = `export const scratch = "${externalToken}"\n`
const siblingExternal = `export const sibling = "external-${Date.now()}"\n`
writeFileSync(scratchAbs, externalContent)
writeFileSync(siblingAbs, siblingExternal)
// Positive-signal wait: block until the sibling (clean) tab has
// demonstrably picked up its external content via the reload
// pipeline. Once that's happened, the dirty tab has had identical
// time to be (incorrectly) reloaded if the guard is broken.
await expect
.poll(async () => getMonacoContent(orcaPage, siblingAbs), {
timeout: 10_000,
message: 'sibling clean tab never reflected external write'
})
.toBe(siblingExternal)
// The guard: the draft must survive, the dirty flag must stay, and
// we must NOT have been marked as tombstoned/renamed. The reload
// pipeline in PR #735 skips the scheduleDebouncedExternalReload
// call when any matching openFile is dirty — verifying the draft
// didn't shift is the strongest store-visible proof that skip
// happened.
const afterWrite = await getOpenFileSummary(orcaPage, fileId!)
expect(afterWrite).toEqual({
isDirty: true,
externalMutation: null,
draft: draftContent
})
// The previous Monaco-level assertion (that the dirty tab's Monaco
// did not contain the external token) is intentionally omitted:
// activating the sibling unmounts the dirty tab's MonacoEditor, so
// its model is no longer observable until we re-activate. The
// draft-survives-in-store check above is the load-bearing contract
// for PR #735's dirty-guard — syncContentOnMount will repaint from
// the draft when the user switches back, which is covered by
// MonacoEditor's existing unit tests for syncContentOnMount.
} finally {
if (existsSync(siblingAbs)) {
try {
unlinkSync(siblingAbs)
} catch {
/* ignore */
}
}
}
} finally {
if (existsSync(scratchAbs)) {
try {
@ -330,7 +394,13 @@ test.describe('External File Change Reflection', () => {
// it fresh for this test (instead of reusing src/index.ts) avoids
// leaving the seeded repo in a broken state if cleanup fails and lets
// tests in the same worker keep depending on the seeded files.
const scratchRel = 'scratch-external-delete.ts'
// The unique timestamp + random suffix isolates parallel workers that
// share TEST_REPO_PATH — without it, two workers executing this spec
// concurrently would race on the same on-disk path and one worker's
// unlinkSync would fire a watcher event in the other worker's session,
// flipping its tombstone prematurely or clearing it via the parallel
// resurrection. Same rationale as tests 1-3.
const scratchRel = `scratch-external-delete-${Date.now()}-${Math.random().toString(36).slice(2, 8)}.ts`
const scratchAbs = path.join(worktreePath!, scratchRel)
const initialContent = 'export const scratch = "initial"\n'
writeFileSync(scratchAbs, initialContent)

30
tests/e2e/helpers/external-file-change.ts
View file

@ -119,15 +119,33 @@ export async function getOpenFileSummary(
* assertions are stable across Monaco upgrades — unlike scraping
* `.view-lines` which targets an internal class name.
*
* Returns null if the editor hasn't mounted yet for this path; callers
* Why scan by model URI instead of registry.get(filePath): the registry is
* keyed by `viewStateKey` (per-pane, unique even across split panes viewing
* the same file) to avoid collisions between sibling panes. Specs not
* exercising split panes only care about "the editor showing this file", so
* we iterate the Map values and match the Monaco model's URI fsPath. If
* multiple panes view the same file the first match is returned, which is
* fine for non-split specs — all panes share the same retained model, so
* `getValue()` returns the same content either way.
*
* Returns null if no mounted editor has a model for this path; callers
* should poll until it becomes a string.
*/
export async function getMonacoContent(page: Page, filePath: string): Promise<string | null> {
return page.evaluate((fp) => {
const registry = (window as unknown as Record<string, unknown>).__monacoEditors as
| Map<string, { getValue: () => string }>
| undefined
const editorInstance = registry?.get(fp)
return editorInstance ? editorInstance.getValue() : null
const registry = window.__monacoEditors
if (!registry) {
return null
}
for (const editorInstance of registry.values()) {
const model = editorInstance.getModel?.()
if (!model) {
continue
}
if (model.uri.fsPath === fp) {
return editorInstance.getValue()
}
}
return null
}, filePath)
}

8
tests/e2e/helpers/runtime-types.ts
View file

@ -1,3 +1,5 @@
import type { editor } from 'monaco-editor'
import type { AppState } from '../../../src/renderer/src/store/types'
import type { OpenFile, RightSidebarTab } from '../../../src/renderer/src/store/slices/editor'
import type { ManagedPane } from '../../../src/renderer/src/lib/pane-manager/pane-manager-types'
@ -45,6 +47,12 @@ declare global {
interface Window {
__store?: AppStore
__paneManagers?: Map<string, PaneManagerLike>
// Why: MonacoEditor.tsx exposes the live editor instances in dev/E2E keyed
// by viewStateKey (one entry per mounted pane, unique even when split panes
// view the same file). Declaring the shape here lets tests call
// `window.__monacoEditors` directly instead of casting through `unknown`,
// so a future shape change is caught at compile time.
__monacoEditors?: Map<string, editor.IStandaloneCodeEditor>
}
}