From 211d4dde3fd1c847635d298270862ff1761b1617 Mon Sep 17 00:00:00 2001 From: wgqqqqq Date: Tue, 22 Sep 2026 16:57:06 +0800 Subject: [PATCH] fix(editor): scope external disk syncs and encoding reloads to one model The external-sync suppression was a process-wide depth counter, so a bracketed disk write on one model also muted dirty tracking for every other model that a content listener edited synchronously. Track the bracket per model instead, and keep emitting the content-changed notification for disk writes even while dirty tracking is suppressed. The encoding reload had no request ownership either: a slow read could overwrite a newer view, a newer encoding selection, or edits made while it was in flight. Guard it with a request id, a live view check, and a buffer version comparison, and commit the decoded buffer with its saved baseline before awaiting file metadata. Cover both paths with the new component test and the extended model manager tests, and record the focused verification command in the editor guide. Co-authored-by: OpenBitFun <318544290+bitfun-ai@users.noreply.github.com> --- src/web-ui/src/tools/editor/AGENTS.md | 10 + .../editor/components/CodeEditor.test.tsx | 340 ++++++++++++++++++ .../tools/editor/components/CodeEditor.tsx | 49 +-- .../services/MonacoModelManager.test.ts | 61 +++- .../editor/services/MonacoModelManager.ts | 77 ++-- 5 files changed, 470 insertions(+), 67 deletions(-) create mode 100644 src/web-ui/src/tools/editor/components/CodeEditor.test.tsx diff --git a/src/web-ui/src/tools/editor/AGENTS.md b/src/web-ui/src/tools/editor/AGENTS.md index a4446ed931..647392272f 100644 --- a/src/web-ui/src/tools/editor/AGENTS.md +++ b/src/web-ui/src/tools/editor/AGENTS.md @@ -30,6 +30,16 @@ This directory follows `src/web-ui/AGENTS.md`. ## Focused verification +For code editor disk synchronization, encoding reloads, and dirty-state changes: + +```bash +pnpm --dir src/web-ui run test:run src/tools/editor/components/CodeEditor.test.tsx src/tools/editor/services/MonacoModelManager.test.ts src/tools/editor/services/EditorDocument.test.ts src/tools/editor/utils/diskFileVersion.test.ts +``` + +The component tests use the real document/model manager with fake Monaco rendering +and host IO. They cover delayed reads, stale requests, surface changes, and view +remounts; they do not establish live SSH or peer transport behavior. + Run from the repository root after Markdown editor changes: ```bash diff --git a/src/web-ui/src/tools/editor/components/CodeEditor.test.tsx b/src/web-ui/src/tools/editor/components/CodeEditor.test.tsx new file mode 100644 index 0000000000..1dec6227d5 --- /dev/null +++ b/src/web-ui/src/tools/editor/components/CodeEditor.test.tsx @@ -0,0 +1,340 @@ +// @vitest-environment jsdom +import React, { act, useState } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { flushSync } from 'react-dom'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type * as Monaco from 'monaco-editor'; +import CodeEditor from './CodeEditor'; +import { EditorDocument, EditorDocumentContext } from '../services/EditorDocument'; +import { monacoModelManager } from '../services/MonacoModelManager'; +import { setMonacoRuntime } from '../services/monacoRuntime'; +import { globalEventBus } from '@/infrastructure/event-bus'; +import { activateSurface } from '@/infrastructure/peer-device/deviceSurface'; + +const mocks = vi.hoisted(() => ({ + initialize: vi.fn(), read: vi.fn(), metadata: vi.fn(), write: vi.fn(), logError: vi.fn(), +})); +vi.mock('../services/MonacoInitManager', () => ({ monacoInitManager: { initialize: mocks.initialize } })); +vi.mock('../services/editorFileAccess', () => ({ + standaloneEditorFileAccess: () => ({ readFileContent: mocks.read, getFileMetadata: mocks.metadata, writeFileContent: mocks.write }), +})); +vi.mock('@/infrastructure/api/service-api/WorkspaceAPI', () => ({ + workspaceAPI: { readWorkspaceFile: mocks.read, getWorkspaceFileMetadata: mocks.metadata, writeWorkspaceFile: mocks.write }, +})); +vi.mock('@/infrastructure/api/service-api/ApiClient', () => ({ api: { invoke: vi.fn(async () => ({})) } })); +vi.mock('@/infrastructure/appearance/adapters/MonacoAppearanceAdapter', () => ({ + monacoAppearanceAdapter: { attachMonaco: () => 'test' }, +})); +vi.mock('../services/ActiveEditTargetService', () => ({ + activeEditTargetService: { bindTarget: () => () => {}, setActiveTarget: vi.fn(), clearActiveTarget: vi.fn() }, + createMonacoEditTarget: () => ({ id: 'test-editor' }), +})); +vi.mock('@/infrastructure/config/services/ConfigManager', () => ({ + configManager: { getConfig: async () => null, watch: () => () => {} }, +})); +vi.mock('@/infrastructure/event-bus', () => { + const listeners = new Map unknown>>(); + return { globalEventBus: { + on: (name: string, listener: (data: unknown) => unknown) => { + if (!listeners.has(name)) listeners.set(name, new Set()); + listeners.get(name)!.add(listener); + return () => listeners.get(name)?.delete(listener); + }, + off: (name: string, listener: (data: unknown) => unknown) => listeners.get(name)?.delete(listener), + emit: async (name: string, data: unknown) => Promise.all([...listeners.get(name) ?? []].map(listener => listener(data))), + } }; +}); +vi.mock('@/infrastructure/i18n', () => ({ useI18n: () => ({ t: (key: string) => key }) })); +vi.mock('@/shared/utils/logger', () => ({ + createLogger: () => ({ debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: mocks.logError }), +})); +vi.mock('@/shared/utils/debugProbe', () => ({ sendDebugProbe: vi.fn() })); +vi.mock('@/infrastructure/confirm-dialog', () => ({ confirmDialog: async () => true })); +vi.mock('./EditorBreadcrumb', () => ({ EditorBreadcrumb: () => null })); +vi.mock('@openbitfun/ui', () => ({ Button: 'button', LoadingState: 'div' })); +vi.mock('./EditorStatusBar', () => ({ + EditorStatusBar: ({ encoding, onEncodingClick }: { encoding: string; onEncodingClick: React.MouseEventHandler }) => ( + + ), +})); +vi.mock('./StatusBarPopovers', () => ({ + GoToLinePopover: () => null, + IndentPopover: () => null, + LanguagePopover: () => null, + EncodingPopover: ({ onConfirm }: { onConfirm: (encoding: string) => Promise }) => ( + <> + + ), +})); + +const disposable = () => ({ dispose() {} }); +class TextModel { + private version = 1; + private listeners = new Set<() => void>(); + private options = { tabSize: 2, insertSpaces: true }; + constructor(private value: string, readonly uri: { toString(): string }) {} + getValue() { return this.value; } + setValue(value: string) { + if (value === this.value) return; + this.value = value; + this.version++; + for (const listener of this.listeners) listener(); + } + getAlternativeVersionId() { return this.version; } + getLanguageId() { return 'plaintext'; } + getOptions() { return this.options; } + updateOptions(options: typeof this.options) { this.options = options; } + onDidChangeOptions = disposable; + onDidChangeContent(listener: () => void) { + this.listeners.add(listener); + return { dispose: () => this.listeners.delete(listener) }; + } + dispose() { this.listeners.clear(); } +} + +function createRuntime() { + const models = new Map(); + return { + Uri: { file: (path: string) => ({ toString: () => `file://${path}` }), parse: (path: string) => ({ toString: () => path }) }, + editor: { + onWillDisposeModel: disposable, + getModel: (uri: { toString(): string }) => models.get(uri.toString()) ?? null, + createModel: (content: string, _language: string, uri: { toString(): string }) => { + const model = new TextModel(content, uri); + models.set(uri.toString(), model); + return model; + }, + create: (container: HTMLElement) => ({ + getDomNode: () => container, + updateOptions() {}, + onDidFocusEditorText: disposable, + onDidBlurEditorText: disposable, + onDidChangeModel: disposable, + onDidChangeCursorPosition: disposable, + onDidChangeCursorSelection: disposable, + onMouseDown: disposable, + onMouseMove: disposable, + onDidLayoutChange: disposable, + getPosition: () => ({ lineNumber: 1, column: 1 }), + setPosition() {}, + saveViewState: () => ({}), + restoreViewState() {}, + dispose() {}, + }), + }, + languages: { getLanguages: () => [] }, + } as unknown as typeof Monaco; +} + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason: Error) => void; + const promise = new Promise((done, fail) => { resolve = done; reject = fail; }); + return { promise, resolve, reject }; +} + +// Exercise the callback that owns the visible dirty indicator as well as the +// real manager and document snapshot; only Monaco rendering and host IO are fake. +function EditorTab({ session, filePath, onChange }: { + session: EditorDocument; filePath: string; onChange?: (content: string) => void; +}) { + const [dirty, setDirty] = useState(session.snapshot?.isDirty ?? false); + return + {dirty ? 'modified' : 'saved'} + { + setDirty(changed); + onChange?.(content); + }} /> + ; +} + +let root: Root; +let container: HTMLDivElement; +let session: EditorDocument; +let serial = 0; +let documents: EditorDocument[]; +const path = '/repo/a.txt'; +const fileMetadata = { isFile: true, size: 4, modified: 1 }; +function model(document = session) { return monacoModelManager.getModel(document.modelKey)!; } +function metadata(document = session) { return monacoModelManager.getModelMetadata(document.modelKey)!; } +async function render(document = session, filePath = path) { + await act(async () => root.render()); + expect(mocks.logError).not.toHaveBeenCalled(); + expect(model(document)).not.toBeNull(); +} +async function reloadEncoding(encoding = 'utf16') { + await act(async () => container.querySelector('[data-testid="encoding"]')!.click()); + await act(async () => container.querySelector(`[data-testid="${encoding}"]`)!.click()); +} +function newDocument(filePath = path) { + const document = new EditorDocument(`sync-${++serial}`, { surfaceId: 'local', workspaceId: 'test-workspace' }, filePath); + document.capture('disk', false); + documents.push(document); + return document; +} + +beforeEach(() => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval'] }); + vi.clearAllMocks(); + activateSurface('local'); + documents = []; + mocks.read.mockReset().mockResolvedValue('disk'); + mocks.metadata.mockReset().mockResolvedValue(fileMetadata); + const runtime = createRuntime(); + setMonacoRuntime(runtime); + mocks.initialize.mockResolvedValue(runtime); + container = document.createElement('div'); + document.body.append(container); + root = createRoot(container); + session = newDocument(); +}); +afterEach(async () => { + await act(async () => root.unmount()); + documents.forEach(document => monacoModelManager.releaseDocumentModel(document.modelKey)); + container.remove(); + setMonacoRuntime(null); + vi.useRealTimers(); +}); + +describe('CodeEditor disk synchronization', () => { + it.each(['success', 'failure'])('keeps edits dirty after a delayed metadata %s', async outcome => { + await render(); + const pendingMetadata = deferred(); + mocks.read.mockResolvedValueOnce('decoded'); + mocks.metadata.mockReturnValueOnce(pendingMetadata.promise); + await reloadEncoding(); + expect(model().getValue()).toBe('decoded'); + expect(metadata().originalContent).toBe('decoded'); + await act(async () => model().setValue('decoded plus user edits')); + await act(async () => { + if (outcome === 'success') pendingMetadata.resolve(fileMetadata); + else pendingMetadata.reject(new Error('offline')); + }); + expect(metadata().isDirty).toBe(true); + expect(metadata().originalContent).toBe('decoded'); + expect(session.snapshot).toMatchObject({ content: 'decoded plus user edits', savedContent: 'decoded', isDirty: true }); + expect(container.querySelector('[data-testid="dirty"]')?.textContent).toBe('modified'); + await act(async () => root.render(null)); + await render(); + expect(metadata().isDirty).toBe(true); + }); + + it('discards an encoding read after this view switches to another document', async () => { + await render(); + const pendingRead = deferred(); + mocks.read.mockReturnValueOnce(pendingRead.promise); + await reloadEncoding(); + const next = newDocument('/repo/b.txt'); + await render(next, '/repo/b.txt'); + await act(async () => model(next).setValue('B draft')); + await act(async () => pendingRead.resolve('A decoded')); + expect(model(next).getValue()).toBe('B draft'); + expect(metadata(next).isDirty).toBe(true); + expect(model().getValue()).toBe('disk'); + expect(next.snapshot).toMatchObject({ content: 'B draft', savedContent: 'disk', isDirty: true }); + }); + + it('keeps the most recent encoding selection when reads finish out of order', async () => { + await render(); + const older = deferred(); + mocks.read.mockReturnValueOnce(older.promise).mockResolvedValueOnce('Latin-1 decoded'); + await reloadEncoding(); + await reloadEncoding('latin1'); + await act(async () => older.resolve('UTF-16 decoded')); + expect(model().getValue()).toBe('Latin-1 decoded'); + expect(metadata().originalContent).toBe('Latin-1 decoded'); + expect(container.querySelector('[data-testid="encoding"]')?.textContent).toBe('ISO-8859-1'); + }); + + it('does not discard edits made while the encoded content is being read', async () => { + await render(); + const pendingRead = deferred(); + mocks.read.mockReturnValueOnce(pendingRead.promise); + await reloadEncoding(); + await act(async () => model().setValue('new draft')); + await act(async () => pendingRead.resolve('decoded')); + expect(model().getValue()).toBe('new draft'); + expect(metadata().isDirty).toBe(true); + expect(container.querySelector('[data-testid="encoding"]')?.textContent).toBe('UTF-8'); + }); + + it('ignores an encoding read completed after closing the editor', async () => { + await render(); + const pendingRead = deferred(); + mocks.read.mockReturnValueOnce(pendingRead.promise); + await reloadEncoding(); + await act(async () => root.render(null)); + await act(async () => pendingRead.resolve('decoded after close')); + expect(mocks.metadata).not.toHaveBeenCalled(); + expect(session.snapshot).toEqual({ content: 'disk', savedContent: 'disk', isDirty: false }); + await render(); + expect(model().getValue()).toBe('disk'); + }); + + it('does not mark a retained document saved when old encoding metadata finishes in another view', async () => { + await render(); + const pendingMetadata = deferred(); + mocks.read.mockResolvedValueOnce('decoded'); + mocks.metadata.mockReturnValueOnce(pendingMetadata.promise); + await reloadEncoding(); + await act(async () => model().setValue('A draft')); + const next = newDocument('/repo/b.txt'); + await act(async () => root.render(null)); + await render(next, '/repo/b.txt'); + await act(async () => model(next).setValue('B draft')); + await act(async () => pendingMetadata.resolve(fileMetadata)); + expect(metadata()).toMatchObject({ originalContent: 'decoded', isDirty: true }); + expect(metadata(next)).toMatchObject({ originalContent: 'disk', isDirty: true }); + expect(model().getValue()).toBe('A draft'); + expect(model(next).getValue()).toBe('B draft'); + }); + + it('preserves the origin document when the active device changes during an encoding read', async () => { + activateSurface('peer-a'); + session = new EditorDocument(`peer-${++serial}`, { surfaceId: 'peer-a', workspaceId: 'peer-workspace' }, path); + documents.push(session); + session.capture('peer content', false); + await render(); + const pendingRead = deferred(); + mocks.read.mockReturnValueOnce(pendingRead.promise); + await reloadEncoding(); + activateSurface('local'); + await act(async () => pendingRead.resolve('old peer response')); + expect(model().getValue()).toBe('peer content'); + expect(session.snapshot).toMatchObject({ content: 'peer content', isDirty: false }); + expect(mocks.metadata).not.toHaveBeenCalled(); + }); + + it('persists a disk snapshot before a content callback synchronously removes the view', async () => { + await act(async () => root.render( { + if (content === 'external content') flushSync(() => root.render(null)); + }} />)); + await act(async () => model().setValue('local draft')); + mocks.read.mockResolvedValueOnce('external content'); + await act(async () => { await globalEventBus.emit('editor:file-changed', { filePath: path }); }); + expect(container.childElementCount).toBe(0); + expect(session.snapshot).toEqual({ content: 'external content', savedContent: 'external content', isDirty: false }); + await render(); + expect(model().getValue()).toBe('external content'); + expect(metadata().isDirty).toBe(false); + expect(container.querySelector('[data-testid="dirty"]')?.textContent).toBe('saved'); + }); + + it('settles an external reload before remounting and clears the tab indicator', async () => { + await render(); + await act(async () => model().setValue('local draft')); + expect(container.querySelector('[data-testid="dirty"]')?.textContent).toBe('modified'); + mocks.read.mockResolvedValueOnce('external content'); + await act(async () => { await globalEventBus.emit('editor:file-changed', { filePath: path }); }); + expect(model().getValue()).toBe('external content'); + expect(metadata()).toMatchObject({ isDirty: false, originalContent: 'external content' }); + expect(session.snapshot).toEqual({ content: 'external content', savedContent: 'external content', isDirty: false }); + expect(container.querySelector('[data-testid="dirty"]')?.textContent).toBe('saved'); + await act(async () => root.render(null)); + await render(); + expect(model().getValue()).toBe('external content'); + expect(metadata().isDirty).toBe(false); + expect(container.querySelector('[data-testid="dirty"]')?.textContent).toBe('saved'); + }); +}); diff --git a/src/web-ui/src/tools/editor/components/CodeEditor.tsx b/src/web-ui/src/tools/editor/components/CodeEditor.tsx index 288c72f713..10f6dce520 100644 --- a/src/web-ui/src/tools/editor/components/CodeEditor.tsx +++ b/src/web-ui/src/tools/editor/components/CodeEditor.tsx @@ -272,6 +272,7 @@ const CodeEditor: React.FC = ({ const modelRef = useRef(null); const isUnmountedRef = useRef(false); const isCheckingFileRef = useRef(false); + const encodingReloadIdRef = useRef(0); /** Last disk state known to match loaded/saved editor content (mtime + size; local + remote). */ const diskVersionRef = useRef(null); const lastReportedMissingRef = useRef(undefined); @@ -373,11 +374,11 @@ const CodeEditor: React.FC = ({ isLoadingContentRef.current = true; // Programmatic disk sync: bracket the write so the model manager does not // flag the model dirty for content nobody typed (issue #3165). - monacoModelManager.beginExternalSync(); + monacoModelManager.beginExternalSync(model); try { model.setValue(nextContent); } finally { - monacoModelManager.endExternalSync(); + monacoModelManager.endExternalSync(model); } setIndentation(applyModelIndentation(model, latestEditorConfigRef.current ?? {}, true)); @@ -418,13 +419,14 @@ const CodeEditor: React.FC = ({ savedVersionIdRef.current = modelRef.current.getAlternativeVersionId(); monacoModelManager.markAsSaved(modelKey); } + documentSession?.capture(fileContent, false); onContentChange?.(fileContent, false); reportFileMissingFromDisk(false); queueMicrotask(() => { isLoadingContentRef.current = false; }); }, - [applyExternalContentToModel, filePath, modelKey, onContentChange, reportFileMissingFromDisk, updateLargeFileMode] + [applyExternalContentToModel, documentSession, filePath, modelKey, onContentChange, reportFileMissingFromDisk, updateLargeFileMode] ); const shouldBlockLargeFileExpansionClick = useCallback((target: EventTarget | null): boolean => { @@ -1038,6 +1040,7 @@ const CodeEditor: React.FC = ({ return () => { cancelled = true; isUnmountedRef.current = true; + encodingReloadIdRef.current += 1; indentationListener?.dispose(); if (modelRef.current === model) modelRef.current = null; clearScheduledNavigationSettlement(); @@ -1405,20 +1408,28 @@ const CodeEditor: React.FC = ({ }, [documentFiles, filePath, isMemoryContent]); const handleEncodingConfirm = useCallback(async (newEncoding: string) => { - if (isMemoryContent) return; - setEncoding(newEncoding); - if (!filePath) return; + const model = modelRef.current; + if (isMemoryContent || !filePath || !model || isUnmountedRef.current) return; + if (documentSession && !documentSession.isCurrent()) return; + const requestId = ++encodingReloadIdRef.current; + const versionBeforeRead = model.getAlternativeVersionId(); + const isCurrentRequest = () => + !isUnmountedRef.current && modelRef.current === model && filePathRef.current === filePath + && encodingReloadIdRef.current === requestId + && (!documentSession || documentSession.isCurrent()); + try { - const workspaceAPI = documentFiles; - const content = await workspaceAPI.readFileContent(filePath, newEncoding); - updateLargeFileMode(content); - setContent(content); - originalContentRef.current = content; - setHasChanges(false); - hasChangesRef.current = false; - applyExternalContentToModel(content); + const content = await documentFiles.readFileContent(filePath, newEncoding); + // A slow read must not overwrite a new view, a newer encoding selection, + // or edits made since the user requested the reload. + if (!isCurrentRequest() || model.getAlternativeVersionId() !== versionBeforeRead) return; + setEncoding(newEncoding); + // Commit the buffer and saved baseline together, before awaiting metadata. + applyDiskSnapshotToEditor(content, null); + if (!isCurrentRequest()) return; try { const fileInfo = await fetchFileMetadata(); + if (!isCurrentRequest()) return; if (isFileMissingFromMetadata(fileInfo)) { reportFileMissingFromDisk(true); } else { @@ -1429,24 +1440,20 @@ const CodeEditor: React.FC = ({ } } } catch (err) { + if (!isCurrentRequest()) return; if (isLikelyFileNotFoundError(err)) { reportFileMissingFromDisk(true); } log.warn('Failed to sync disk version after encoding change', err); } - // Same unmount race as the disk-sync path: settle the saved state - // synchronously right after the bracketed content write (issue #3165). - if (modelRef.current) { - savedVersionIdRef.current = modelRef.current.getAlternativeVersionId(); - monacoModelManager.markAsSaved(modelKey); - } } catch (err) { + if (!isCurrentRequest()) return; if (isLikelyFileNotFoundError(err)) { reportFileMissingFromDisk(true); } log.warn('Failed to reload file with new encoding', err); } - }, [applyExternalContentToModel, documentFiles, fetchFileMetadata, filePath, isMemoryContent, modelKey, reportFileMissingFromDisk, updateLargeFileMode]); + }, [applyDiskSnapshotToEditor, documentFiles, documentSession, fetchFileMetadata, filePath, isMemoryContent, reportFileMissingFromDisk]); const handleLanguageConfirm = useCallback((languageId: string) => { userLanguageOverrideRef.current = true; diff --git a/src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts b/src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts index ec5eb0c6b3..8082e61ef5 100644 --- a/src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts +++ b/src/web-ui/src/tools/editor/services/MonacoModelManager.test.ts @@ -106,6 +106,61 @@ afterEach(() => { }); describe('MonacoModelManager external sync dirty state (issue #3165)', () => { + it('notifies content subscribers during disk sync and tracks edits to other models', () => { + const filePath = '/repo/content-subscriber.ts'; + monacoModelManager.getOrCreateModel(filePath, 'typescript', 'before'); + const otherPath = '/repo/content-subscriber-other.ts'; + const other = monacoModelManager.getOrCreateModel(otherPath, 'typescript', 'saved'); + const contents: string[] = []; + const unsubscribe = monacoModelManager.onModelContentChanged(event => { + if (event.filePath !== filePath) return; + contents.push(event.content); + other.setValue('unsaved edit from a content listener'); + }); + try { + monacoModelManager.updateModelContent(filePath, 'disk update', true); + expect(contents).toEqual(['disk update']); + expect(monacoModelManager.getModelMetadata(filePath)?.isDirty).toBe(false); + expect(monacoModelManager.getModelMetadata(otherPath)?.isDirty).toBe(true); + expect(dirtyListener.events.filter(event => event.filePath === filePath)).toEqual([ + { filePath, isDirty: false }, + ]); + } finally { + unsubscribe(); + } + }); + + it('initializes a reused empty model without a transient dirty event', () => { + const filePath = '/repo/reused-empty.ts'; + const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', ''); + const reused = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'disk content'); + expect(reused).toBe(model); + expect(monacoModelManager.getModelMetadata(filePath)).toMatchObject({ + originalContent: 'disk content', isDirty: false, + }); + expect(dirtyListener.events.filter(event => event.filePath === filePath)).toEqual([ + { filePath, isDirty: false }, + ]); + }); + + it('keeps nested sync suppression until the outer bracket closes and restores it after failure', () => { + const filePath = '/repo/nested-sync.ts'; + const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'saved'); + expect(() => { + monacoModelManager.beginExternalSync(model); + try { + monacoModelManager.updateModelContent(filePath, 'first sync', true); + model.setValue('outer sync'); + expect(monacoModelManager.getModelMetadata(filePath)?.isDirty).toBe(false); + throw new Error('sync failed'); + } finally { + monacoModelManager.endExternalSync(model); + } + }).toThrow('sync failed'); + model.setValue('user edit'); + expect(monacoModelManager.getModelMetadata(filePath)?.isDirty).toBe(true); + }); + it('updateModelContent with markAsSaved=true leaves the model clean', () => { const filePath = '/repo/external-sync-saved.ts'; const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const a = 1;'); @@ -140,11 +195,11 @@ describe('MonacoModelManager external sync dirty state (issue #3165)', () => { const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const c = 1;'); dirtyListener.events.length = 0; - monacoModelManager.beginExternalSync(); + monacoModelManager.beginExternalSync(model); try { model.setValue('const c = 2;'); } finally { - monacoModelManager.endExternalSync(); + monacoModelManager.endExternalSync(model); } monacoModelManager.markAsSaved(filePath); @@ -171,7 +226,7 @@ describe('MonacoModelManager external sync dirty state (issue #3165)', () => { const filePath = '/repo/unbalanced-bracket.ts'; const model = monacoModelManager.getOrCreateModel(filePath, 'typescript', 'const e = 1;'); - monacoModelManager.endExternalSync(); // no matching begin: must be a no-op + monacoModelManager.endExternalSync(model); // no matching begin: must be a no-op dirtyListener.events.length = 0; model.setValue('const e = 2; // user typed'); diff --git a/src/web-ui/src/tools/editor/services/MonacoModelManager.ts b/src/web-ui/src/tools/editor/services/MonacoModelManager.ts index f468f1cc48..285c4e1d32 100644 --- a/src/web-ui/src/tools/editor/services/MonacoModelManager.ts +++ b/src/web-ui/src/tools/editor/services/MonacoModelManager.ts @@ -84,12 +84,11 @@ class MonacoModelManager { private globalListenersInstalled = false; /** - * Depth counter for programmatic external-sync brackets. While non-zero, - * content changes are disk-sync writes (issue #3165), so the dirty state is - * owned by the sync caller (markAsSaved / updateModelContent) instead of the - * change listener. A counter keeps nested brackets balanced. + * Suppress dirty recomputation only for the model being synchronized. + * Content listeners may synchronously edit other models; those edits still + * participate in dirty tracking. Depth supports nested writes to one model. */ - private externalSyncDepth = 0; + private externalSyncDepth = new WeakMap(); private constructor() {} @@ -155,14 +154,7 @@ class MonacoModelManager { } if (initialContent && model.getValue() === '') { - model.setValue(initialContent); - - const metadata = this.modelMetadata.get(uriString); - if (metadata) { - metadata.savedVersionId = model.getAlternativeVersionId(); - metadata.originalContent = initialContent; - metadata.isDirty = false; - } + this.updateModelContent(modelKey, initialContent, true); } return model; @@ -221,27 +213,23 @@ class MonacoModelManager { model: monaco.editor.ITextModel ): void { const listener = model.onDidChangeContent(() => { - // Inside an external-sync bracket the write is the disk truth, not a user - // edit: skip the dirty recompute and the transient dirty broadcast so the - // tab never flashes "modified" for a programmatic sync (issue #3165). - // The bracket caller settles the final state (markAsSaved / saved flag). - if (this.externalSyncDepth > 0) { - return; - } const metadata = this.modelMetadata.get(uriString); if (metadata) { - const currentVersionId = model.getAlternativeVersionId(); - metadata.isDirty = this.documentModels.has(uriString) ? model.getValue() !== metadata.originalContent - : currentVersionId !== metadata.savedVersionId; - - window.dispatchEvent(new CustomEvent('monaco-model-dirty-changed', { - detail: { - uri: uriString, - filePath: metadata.filePath, - isDirty: metadata.isDirty - } - })); - + if (!this.externalSyncDepth.has(model)) { + const currentVersionId = model.getAlternativeVersionId(); + metadata.isDirty = this.documentModels.has(uriString) ? model.getValue() !== metadata.originalContent + : currentVersionId !== metadata.savedVersionId; + + window.dispatchEvent(new CustomEvent('monaco-model-dirty-changed', { + detail: { + uri: uriString, + filePath: metadata.filePath, + isDirty: metadata.isDirty + } + })); + } + + // Disk writes change content too, even when dirty tracking is suppressed. this.emitModelContentChanged({ uri: uriString, filePath: metadata.filePath, @@ -364,13 +352,13 @@ class MonacoModelManager { // #3165): bracket the write so the change listener does not recompute the // dirty flag or broadcast a transient "modified" state in between. if (markAsSaved) { - this.beginExternalSync(); + this.beginExternalSync(model); } try { model.setValue(content); } finally { if (markAsSaved) { - this.endExternalSync(); + this.endExternalSync(model); } } @@ -406,21 +394,24 @@ class MonacoModelManager { } /** - * Open a programmatic external-sync bracket. While it is open, model content - * changes skip the dirty recompute and the transient dirty broadcast + * Open a programmatic external-sync bracket for one model. While it is open, + * its changes skip the dirty recompute and the transient dirty broadcast * (issue #3165). Pair every begin with an end in a finally block. */ - public beginExternalSync(): void { - this.externalSyncDepth += 1; + public beginExternalSync(model: monaco.editor.ITextModel): void { + this.externalSyncDepth.set(model, (this.externalSyncDepth.get(model) ?? 0) + 1); } /** - * Close an external-sync bracket. Unbalanced calls (more ends than begins) - * are no-ops so suppression can never get stuck on and swallow real edits. + * Close a bracket for this model. Extra ends are no-ops; callers must still + * pair every begin with an end in a finally block. */ - public endExternalSync(): void { - if (this.externalSyncDepth > 0) { - this.externalSyncDepth -= 1; + public endExternalSync(model: monaco.editor.ITextModel): void { + const depth = this.externalSyncDepth.get(model) ?? 0; + if (depth > 1) { + this.externalSyncDepth.set(model, depth - 1); + } else { + this.externalSyncDepth.delete(model); } }