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); } }